From 2453e4b6fbc4e605f4a52e68905ba9b2b9a7c377 Mon Sep 17 00:00:00 2001
From: Oren Milman <orenmn@gmail.com>
Date: Mon, 9 Oct 2017 17:00:15 +0300
Subject: [PATCH 01/11] init commit

---
 Lib/sqlite3/test/regression.py                | 20 +++++++
 .../2017-10-09-16-58-33.bpo-31734.HCa6O5.rst  |  2 +
 Modules/_sqlite/cache.c                       | 58 +++++++++++--------
 3 files changed, 55 insertions(+), 25 deletions(-)
 create mode 100644 Misc/NEWS.d/next/Core and Builtins/2017-10-09-16-58-33.bpo-31734.HCa6O5.rst

diff --git a/Lib/sqlite3/test/regression.py b/Lib/sqlite3/test/regression.py
index 7dd0050528f8fa..bd980d199ba055 100644
--- a/Lib/sqlite3/test/regression.py
+++ b/Lib/sqlite3/test/regression.py
@@ -24,6 +24,7 @@
 import datetime
 import unittest
 import sqlite3 as sqlite
+from test import support
 
 class RegressionTests(unittest.TestCase):
     def setUp(self):
@@ -376,6 +377,25 @@ def CheckCommitCursorReset(self):
                 counter += 1
         self.assertEqual(counter, 3, "should have returned exactly three rows")
 
+    @support.cpython_only
+    def CheckUninitializedCache(self):
+        # bpo-31734: Cache.get() shouldn't crash in case the Cache object is
+        # uninitialized.
+        cache = sqlite.Cache.__new__(sqlite.Cache)
+        self.assertRaises(ValueError, cache.get, None)
+
+    @support.cpython_only
+    def CheckPartiallyInitializedCache(self):
+        # bpo-31734: A failure of the __init__() method of an already
+        # initialized Cache object shouldn't result in the Cache object being
+        # partially initialized, which would cause its get() method to raise a
+        # SystemError.
+        cache = sqlite.Cache(str)
+        try:
+            cache.__init__()
+        except TypeError:
+            pass
+        cache.get(None)
 
 def suite():
     regression_suite = unittest.makeSuite(RegressionTests, "Check")
diff --git a/Misc/NEWS.d/next/Core and Builtins/2017-10-09-16-58-33.bpo-31734.HCa6O5.rst b/Misc/NEWS.d/next/Core and Builtins/2017-10-09-16-58-33.bpo-31734.HCa6O5.rst
new file mode 100644
index 00000000000000..d0c261ec75281d
--- /dev/null
+++ b/Misc/NEWS.d/next/Core and Builtins/2017-10-09-16-58-33.bpo-31734.HCa6O5.rst	
@@ -0,0 +1,2 @@
+Raise a `ValueError` instead of crashing, when the `get()` method of an
+uninitialized `sqlite3.Cache` object is called. Patch by Oren Milman.
diff --git a/Modules/_sqlite/cache.c b/Modules/_sqlite/cache.c
index 72b1f2c5614bf4..094439c9c015a0 100644
--- a/Modules/_sqlite/cache.c
+++ b/Modules/_sqlite/cache.c
@@ -54,13 +54,24 @@ void pysqlite_node_dealloc(pysqlite_Node* self)
     Py_TYPE(self)->tp_free((PyObject*)self);
 }
 
+static void deallocate_nodes(pysqlite_Cache* self) {
+    pysqlite_Node* node;
+    pysqlite_Node* delete_node;
+
+    /* iterate over all nodes and deallocate them */
+    node = self->first;
+    while (node) {
+        delete_node = node;
+        node = node->next;
+        Py_DECREF(delete_node);
+    }
+}
+
 int pysqlite_cache_init(pysqlite_Cache* self, PyObject* args, PyObject* kwargs)
 {
     PyObject* factory;
     int size = 10;
 
-    self->factory = NULL;
-
     if (!PyArg_ParseTuple(args, "O|i", &factory, &size)) {
         return -1;
     }
@@ -70,44 +81,36 @@ int pysqlite_cache_init(pysqlite_Cache* self, PyObject* args, PyObject* kwargs)
         size = 5;
     }
     self->size = size;
+    deallocate_nodes(self);
     self->first = NULL;
     self->last = NULL;
 
-    self->mapping = PyDict_New();
+    Py_XSETREF(self->mapping, PyDict_New());
     if (!self->mapping) {
         return -1;
     }
 
     Py_INCREF(factory);
-    self->factory = factory;
-
-    self->decref_factory = 1;
+    if (self->decref_factory) {
+        Py_SETREF(self->factory, factory);
+    }
+    else {
+        self->factory = factory;
+        self->decref_factory = 1;
+    }
 
     return 0;
 }
 
 void pysqlite_cache_dealloc(pysqlite_Cache* self)
 {
-    pysqlite_Node* node;
-    pysqlite_Node* delete_node;
-
-    if (!self->factory) {
-        /* constructor failed, just get out of here */
-        return;
-    }
-
-    /* iterate over all nodes and deallocate them */
-    node = self->first;
-    while (node) {
-        delete_node = node;
-        node = node->next;
-        Py_DECREF(delete_node);
-    }
-
-    if (self->decref_factory) {
-        Py_DECREF(self->factory);
+    if (self->factory) {
+        deallocate_nodes(self);
+        if (self->decref_factory) {
+            Py_DECREF(self->factory);
+        }
+        Py_DECREF(self->mapping);
     }
-    Py_DECREF(self->mapping);
 
     Py_TYPE(self)->tp_free((PyObject*)self);
 }
@@ -119,6 +122,11 @@ PyObject* pysqlite_cache_get(pysqlite_Cache* self, PyObject* args)
     pysqlite_Node* ptr;
     PyObject* data;
 
+    if (self->factory == NULL) {
+        PyErr_SetString(PyExc_ValueError,
+                        MODULE_NAME ".Cache.__init__() not called");
+        return NULL;
+    }
     node = (pysqlite_Node*)PyDict_GetItem(self->mapping, key);
     if (node) {
         /* an entry for this key already exists in the cache */

From a6aa5082f2c462cb0c6f73c95a05fe7060f397bd Mon Sep 17 00:00:00 2001
From: Oren Milman <orenmn@gmail.com>
Date: Mon, 9 Oct 2017 17:39:47 +0300
Subject: [PATCH 02/11] add a comment to clarify a test.

---
 Lib/sqlite3/test/regression.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Lib/sqlite3/test/regression.py b/Lib/sqlite3/test/regression.py
index bd980d199ba055..20a5309fc54b74 100644
--- a/Lib/sqlite3/test/regression.py
+++ b/Lib/sqlite3/test/regression.py
@@ -395,7 +395,7 @@ def CheckPartiallyInitializedCache(self):
             cache.__init__()
         except TypeError:
             pass
-        cache.get(None)
+        cache.get(None)  # Shouldn't raise a SystemError
 
 def suite():
     regression_suite = unittest.makeSuite(RegressionTests, "Check")

From df768be137c17585e50a5d707e8b454fcb633f46 Mon Sep 17 00:00:00 2001
From: Oren Milman <orenmn@gmail.com>
Date: Mon, 9 Oct 2017 17:42:12 +0300
Subject: [PATCH 03/11] add a blank line

---
 Lib/sqlite3/test/regression.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Lib/sqlite3/test/regression.py b/Lib/sqlite3/test/regression.py
index 20a5309fc54b74..fe419ee7d93819 100644
--- a/Lib/sqlite3/test/regression.py
+++ b/Lib/sqlite3/test/regression.py
@@ -397,6 +397,7 @@ def CheckPartiallyInitializedCache(self):
             pass
         cache.get(None)  # Shouldn't raise a SystemError
 
+
 def suite():
     regression_suite = unittest.makeSuite(RegressionTests, "Check")
     return unittest.TestSuite((regression_suite,))

From 0609f6929093a7d3e6ea853ddadda3188a34c58e Mon Sep 17 00:00:00 2001
From: Oren Milman <orenmn@gmail.com>
Date: Mon, 9 Oct 2017 18:26:57 +0300
Subject: [PATCH 04/11] deallocate the nodes before any change to the Cache
 object.

---
 Modules/_sqlite/cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Modules/_sqlite/cache.c b/Modules/_sqlite/cache.c
index 094439c9c015a0..56e2f1b865828f 100644
--- a/Modules/_sqlite/cache.c
+++ b/Modules/_sqlite/cache.c
@@ -76,12 +76,12 @@ int pysqlite_cache_init(pysqlite_Cache* self, PyObject* args, PyObject* kwargs)
         return -1;
     }
 
+    deallocate_nodes(self);
     /* minimum cache size is 5 entries */
     if (size < 5) {
         size = 5;
     }
     self->size = size;
-    deallocate_nodes(self);
     self->first = NULL;
     self->last = NULL;
 

From 99138bec57a21cd19da2bc3eba8367b563fd1d58 Mon Sep 17 00:00:00 2001
From: Oren Milman <orenmn@gmail.com>
Date: Mon, 9 Oct 2017 18:31:16 +0300
Subject: [PATCH 05/11] add a function comment to deallocate_nodes()

---
 Modules/_sqlite/cache.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Modules/_sqlite/cache.c b/Modules/_sqlite/cache.c
index 56e2f1b865828f..51534828da3e3a 100644
--- a/Modules/_sqlite/cache.c
+++ b/Modules/_sqlite/cache.c
@@ -54,11 +54,12 @@ void pysqlite_node_dealloc(pysqlite_Node* self)
     Py_TYPE(self)->tp_free((PyObject*)self);
 }
 
+/* Iterate over all nodes and deallocate them. If there aren't any nodes, do
+   nothing. */
 static void deallocate_nodes(pysqlite_Cache* self) {
     pysqlite_Node* node;
     pysqlite_Node* delete_node;
 
-    /* iterate over all nodes and deallocate them */
     node = self->first;
     while (node) {
         delete_node = node;

From a31d094cb96d8ec35c218823bd081cd79e6805c5 Mon Sep 17 00:00:00 2001
From: Oren Milman <orenmn@gmail.com>
Date: Mon, 9 Oct 2017 19:33:08 +0300
Subject: [PATCH 06/11] a tiny style improvement

---
 Modules/_sqlite/cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Modules/_sqlite/cache.c b/Modules/_sqlite/cache.c
index 51534828da3e3a..c272f6c8e31a5e 100644
--- a/Modules/_sqlite/cache.c
+++ b/Modules/_sqlite/cache.c
@@ -105,7 +105,7 @@ int pysqlite_cache_init(pysqlite_Cache* self, PyObject* args, PyObject* kwargs)
 
 void pysqlite_cache_dealloc(pysqlite_Cache* self)
 {
-    if (self->factory) {
+    if (self->factory != NULL) {
         deallocate_nodes(self);
         if (self->decref_factory) {
             Py_DECREF(self->factory);

From 52926840851fab8b836192a4ac0b252442c375b3 Mon Sep 17 00:00:00 2001
From: Oren Milman <orenmn@gmail.com>
Date: Mon, 9 Oct 2017 20:23:26 +0300
Subject: [PATCH 07/11] simplify the C code, and change the test accordingly.

---
 Lib/sqlite3/test/regression.py |  9 ++++-----
 Modules/_sqlite/cache.c        | 23 ++++++++++++++---------
 2 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/Lib/sqlite3/test/regression.py b/Lib/sqlite3/test/regression.py
index fe419ee7d93819..2d42026762137d 100644
--- a/Lib/sqlite3/test/regression.py
+++ b/Lib/sqlite3/test/regression.py
@@ -387,15 +387,14 @@ def CheckUninitializedCache(self):
     @support.cpython_only
     def CheckPartiallyInitializedCache(self):
         # bpo-31734: A failure of the __init__() method of an already
-        # initialized Cache object shouldn't result in the Cache object being
-        # partially initialized, which would cause its get() method to raise a
-        # SystemError.
+        # initialized Cache object shouldn't cause the Cache object to be
+        # partially initialized, and its get() method to raise a SystemError.
         cache = sqlite.Cache(str)
         try:
-            cache.__init__()
+            cache.__init__()  # invalid number of arguments
         except TypeError:
             pass
-        cache.get(None)  # Shouldn't raise a SystemError
+        self.assertRaises(ValueError, cache.get, None)
 
 
 def suite():
diff --git a/Modules/_sqlite/cache.c b/Modules/_sqlite/cache.c
index c272f6c8e31a5e..5ccd933f44025d 100644
--- a/Modules/_sqlite/cache.c
+++ b/Modules/_sqlite/cache.c
@@ -73,11 +73,20 @@ int pysqlite_cache_init(pysqlite_Cache* self, PyObject* args, PyObject* kwargs)
     PyObject* factory;
     int size = 10;
 
+    if (self->decref_factory) {
+        self->decref_factory = 0;
+        Py_CLEAR(self->factory);
+    }
+    else {
+        self->factory = NULL;
+    }
+    deallocate_nodes(self);
+    Py_CLEAR(self->mapping);
+
     if (!PyArg_ParseTuple(args, "O|i", &factory, &size)) {
         return -1;
     }
 
-    deallocate_nodes(self);
     /* minimum cache size is 5 entries */
     if (size < 5) {
         size = 5;
@@ -86,19 +95,15 @@ int pysqlite_cache_init(pysqlite_Cache* self, PyObject* args, PyObject* kwargs)
     self->first = NULL;
     self->last = NULL;
 
-    Py_XSETREF(self->mapping, PyDict_New());
+    self->mapping = PyDict_New();
     if (!self->mapping) {
         return -1;
     }
 
     Py_INCREF(factory);
-    if (self->decref_factory) {
-        Py_SETREF(self->factory, factory);
-    }
-    else {
-        self->factory = factory;
-        self->decref_factory = 1;
-    }
+    self->factory = factory;
+
+    self->decref_factory = 1;
 
     return 0;
 }

From e91d28adc2b048fd617ecd52629796c2bec8482e Mon Sep 17 00:00:00 2001
From: Oren Milman <orenmn@gmail.com>
Date: Mon, 9 Oct 2017 20:53:50 +0300
Subject: [PATCH 08/11] fix a bug in the C code

---
 Modules/_sqlite/cache.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Modules/_sqlite/cache.c b/Modules/_sqlite/cache.c
index 5ccd933f44025d..506f4c3da38f96 100644
--- a/Modules/_sqlite/cache.c
+++ b/Modules/_sqlite/cache.c
@@ -81,6 +81,8 @@ int pysqlite_cache_init(pysqlite_Cache* self, PyObject* args, PyObject* kwargs)
         self->factory = NULL;
     }
     deallocate_nodes(self);
+    self->first = NULL;
+    self->last = NULL;
     Py_CLEAR(self->mapping);
 
     if (!PyArg_ParseTuple(args, "O|i", &factory, &size)) {
@@ -92,8 +94,6 @@ int pysqlite_cache_init(pysqlite_Cache* self, PyObject* args, PyObject* kwargs)
         size = 5;
     }
     self->size = size;
-    self->first = NULL;
-    self->last = NULL;
 
     self->mapping = PyDict_New();
     if (!self->mapping) {

From 2c16af6d1f480811b11c4b300a401ad1d611aab2 Mon Sep 17 00:00:00 2001
From: Oren Milman <orenmn@gmail.com>
Date: Mon, 9 Oct 2017 22:35:32 +0300
Subject: [PATCH 09/11] fix a bug in the last fix to the C code

---
 Modules/_sqlite/cache.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Modules/_sqlite/cache.c b/Modules/_sqlite/cache.c
index 506f4c3da38f96..83aa481d77038b 100644
--- a/Modules/_sqlite/cache.c
+++ b/Modules/_sqlite/cache.c
@@ -61,6 +61,8 @@ static void deallocate_nodes(pysqlite_Cache* self) {
     pysqlite_Node* delete_node;
 
     node = self->first;
+    self->first = NULL;
+    self->last = NULL;
     while (node) {
         delete_node = node;
         node = node->next;
@@ -81,8 +83,6 @@ int pysqlite_cache_init(pysqlite_Cache* self, PyObject* args, PyObject* kwargs)
         self->factory = NULL;
     }
     deallocate_nodes(self);
-    self->first = NULL;
-    self->last = NULL;
     Py_CLEAR(self->mapping);
 
     if (!PyArg_ParseTuple(args, "O|i", &factory, &size)) {

From 336edfc1fc192babba3603d353ed757f0913c854 Mon Sep 17 00:00:00 2001
From: Oren Milman <orenmn@gmail.com>
Date: Mon, 9 Oct 2017 22:47:21 +0300
Subject: [PATCH 10/11] improve 2nd test's name

---
 Lib/sqlite3/test/regression.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Lib/sqlite3/test/regression.py b/Lib/sqlite3/test/regression.py
index 2d42026762137d..8fd75702274022 100644
--- a/Lib/sqlite3/test/regression.py
+++ b/Lib/sqlite3/test/regression.py
@@ -385,7 +385,7 @@ def CheckUninitializedCache(self):
         self.assertRaises(ValueError, cache.get, None)
 
     @support.cpython_only
-    def CheckPartiallyInitializedCache(self):
+    def Check__init__Fail(self):
         # bpo-31734: A failure of the __init__() method of an already
         # initialized Cache object shouldn't cause the Cache object to be
         # partially initialized, and its get() method to raise a SystemError.

From f213896359b1d13d2b2ad0960d2713a7fd3681f9 Mon Sep 17 00:00:00 2001
From: Oren Milman <orenmn@gmail.com>
Date: Mon, 9 Oct 2017 22:54:01 +0300
Subject: [PATCH 11/11] function definition style fix

---
 Modules/_sqlite/cache.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Modules/_sqlite/cache.c b/Modules/_sqlite/cache.c
index 83aa481d77038b..1cb883240752dd 100644
--- a/Modules/_sqlite/cache.c
+++ b/Modules/_sqlite/cache.c
@@ -56,7 +56,8 @@ void pysqlite_node_dealloc(pysqlite_Node* self)
 
 /* Iterate over all nodes and deallocate them. If there aren't any nodes, do
    nothing. */
-static void deallocate_nodes(pysqlite_Cache* self) {
+static void deallocate_nodes(pysqlite_Cache* self)
+{
     pysqlite_Node* node;
     pysqlite_Node* delete_node;