Handle allocation failures in cipher registration and parameter table cloning

.github/workflows/ci4sqlite3mc.yml

@@ -57,6 +57,11 @@ jobs:
         ./tempfiletest 20000 1 db4096
         ./tempfiletest 20000 1 chunk faults
 
+    - name: Allocation failure tests
+      run: |
+        make oomtest
+        ./oomtest
+
   android_build:
     name: Android (API ${{ matrix.api-level }})
     runs-on: ubuntu-latest

Makefile.am

@@ -164,8 +164,8 @@ sqlite3shell_LDFLAGS += -no-install
 
 endif
 
-# Tests for the cryptographic primitives (not installed, built like the shell).
-check_PROGRAMS = cryptotest tempfiletest
+# Test programs (not installed, built like the shell).
+check_PROGRAMS = cryptotest tempfiletest oomtest
 cryptotest_SOURCES = test/cryptotest.c
 cryptotest_CFLAGS = $(sqlite3shell_CFLAGS)
 cryptotest_LDADD = $(sqlite3shell_LDADD)
@@ -174,3 +174,7 @@ tempfiletest_SOURCES = test/tempfiletest.c
 tempfiletest_CFLAGS = $(sqlite3shell_CFLAGS)
 tempfiletest_LDADD = $(sqlite3shell_LDADD)
 tempfiletest_LDFLAGS = $(sqlite3shell_LDFLAGS)
+oomtest_SOURCES = test/oomtest.c
+oomtest_CFLAGS = $(sqlite3shell_CFLAGS)
+oomtest_LDADD = $(sqlite3shell_LDADD)
+oomtest_LDFLAGS = $(sqlite3shell_LDFLAGS)

src/cipher_common.c

@@ -67,63 +67,39 @@ sqlite3mcCloneCodecParameterTable()
   /* Count number of codecs and cipher parameters */
   int nTables = 0;
   int nParams = 0;
-  int j, k, n;
+  int j, n;
+  int offset = 0;
   CipherParams* cloneCipherParams;
   CodecParameter* cloneCodecParams;
 
   for (j = 0; globalCodecParameterTable[j].m_name[0] != 0; ++j)
   {
     CipherParams* params = globalCodecParameterTable[j].m_params;
-    for (k = 0; params[k].m_name[0] != 0; ++k);
-    nParams += k;
+    for (n = 0; params[n].m_name[0] != 0; ++n);
+    nParams += n;
   }
   nTables = j;
 
-  /* Allocate memory for cloned codec parameter tables (including sentinel for each table) */
-  cloneCipherParams = (CipherParams*) sqlite3_malloc((nParams + nTables) * sizeof(CipherParams));
-  cloneCodecParams = (CodecParameter*) sqlite3_malloc((nTables + 1) * sizeof(CodecParameter));
+  /* The table array and all parameter arrays, each with its sentinel, share one allocation */
+  cloneCodecParams = (CodecParameter*) sqlite3_malloc((nTables + 1) * sizeof(CodecParameter) +
+                                                      (nParams + nTables) * sizeof(CipherParams));
+  if (cloneCodecParams == NULL)
+    return NULL;
+  cloneCipherParams = (CipherParams*) &cloneCodecParams[nTables + 1];
 
-  /* Create copy of tables */
-  if (cloneCodecParams != NULL)
-  {
-    int offset = 0;
-    for (j = 0; j < nTables; ++j)
-    {
-      CipherParams* params = globalCodecParameterTable[j].m_params;
-      cloneCodecParams[j].m_name = globalCodecParameterTable[j].m_name;
-      cloneCodecParams[j].m_id = globalCodecParameterTable[j].m_id;
-      cloneCodecParams[j].m_params = &cloneCipherParams[offset];
-      for (n = 0; params[n].m_name[0] != 0; ++n);
-      /* Copy all parameters of the current table (including sentinel) */
-      for (k = 0; k <= n; ++k)
-      {
-        cloneCipherParams[offset + k].m_name     = params[k].m_name;
-        cloneCipherParams[offset + k].m_value    = params[k].m_value;
-        cloneCipherParams[offset + k].m_default  = params[k].m_default;
-        cloneCipherParams[offset + k].m_minValue = params[k].m_minValue;
-        cloneCipherParams[offset + k].m_maxValue = params[k].m_maxValue;
-      }
-      offset += (n + 1);
-    }
-    cloneCodecParams[nTables].m_name = globalCodecParameterTable[nTables].m_name;
-    cloneCodecParams[nTables].m_id = globalCodecParameterTable[nTables].m_id;
-    cloneCodecParams[nTables].m_params = NULL;
-  }
-  else
+  for (j = 0; j < nTables; ++j)
   {
-    sqlite3_free(cloneCipherParams);
+    CipherParams* params = globalCodecParameterTable[j].m_params;
+    for (n = 0; params[n].m_name[0] != 0; ++n);
+    memcpy(&cloneCipherParams[offset], params, (n + 1) * sizeof(CipherParams));
+    cloneCodecParams[j] = globalCodecParameterTable[j];
+    cloneCodecParams[j].m_params = &cloneCipherParams[offset];
+    offset += n + 1;
   }
+  cloneCodecParams[nTables] = globalCodecParameterTable[nTables];
   return cloneCodecParams;
 }
 
-SQLITE_PRIVATE void
-sqlite3mcFreeCodecParameterTable(void* ptr)
-{
-  CodecParameter* codecParams = (CodecParameter*)ptr;
-  sqlite3_free(codecParams[0].m_params);
-  sqlite3_free(codecParams);
-}
-
 static const CipherDescriptor mcSentinelDescriptor =
 {
   "", NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL

src/sqlite3mc.c

@@ -426,10 +426,9 @@ mcRegisterCodecExtensions(sqlite3* db, char** pzErrMsg, const sqlite3_api_routin
   rc = (codecParameterTable != NULL) ? SQLITE_OK : SQLITE_NOMEM;
   if (rc == SQLITE_OK)
   {
-    sqlite3_set_clientdata(db, globalConfigTableName, codecParameterTable, sqlite3mcFreeCodecParameterTable);
+    /* On failure this frees the table */
+    rc = sqlite3_set_clientdata(db, globalConfigTableName, codecParameterTable, sqlite3_free);
   }
-
-  rc = (codecParameterTable != NULL) ? SQLITE_OK : SQLITE_NOMEM;
   if (rc == SQLITE_OK)
   {
     rc = sqlite3_create_function(db, "sqlite3mc_config", 1, SQLITE_UTF8 | SQLITE_DETERMINISTIC,
@@ -513,11 +512,23 @@ sqlite3mcGetGlobalCipherCount()
   return cipherCount;
 }
 
+static void
+mcFreeCipherParams(CipherParams* params)
+{
+  int k;
+  for (k = 0; params[k].m_name[0] != 0; ++k)
+  {
+    sqlite3_free((char*) params[k].m_name);
+  }
+  sqlite3_free(params);
+}
+
 static int
 sqlite3mcRegisterCipher(const CipherDescriptor* desc, const CipherParams* params, int makeDefault)
 {
-  int rc;
   int np;
+  int n;
+  char* cipherName;
   CipherParams* cipherParams;
 
   /* Sanity checks */
@@ -567,60 +578,55 @@ sqlite3mcRegisterCipher(const CipherDescriptor* desc, const CipherParams* params
 
   /* Sanity checks were successful, now register cipher */
 
+  if (globalCipherCount >= CODEC_COUNT_MAX)
+    return SQLITE_NOMEM;
+
   cipherParams = (CipherParams*) sqlite3_malloc((np+1) * sizeof(CipherParams));
   if (!cipherParams)
     return SQLITE_NOMEM;
 
-  /* Check for */
-  if (globalCipherCount < CODEC_COUNT_MAX)
+  /* Copy parameters before touching the global tables, so that a failure leaves them unchanged */
+  for (n = 0; n < np; ++n)
   {
-    int n;
-    char* cipherName;
-    ++globalCipherCount;
-    cipherName = globalCipherNameTable[globalCipherCount].m_name;
-    strcpy(cipherName, desc->m_name);
+    cipherParams[n] = params[n];
+    cipherParams[n].m_name = sqlite3_mprintf("%s", params[n].m_name);
+    if (!cipherParams[n].m_name)
+    {
+      cipherParams[n].m_name = globalSentinelName;
+      mcFreeCipherParams(cipherParams);
+      return SQLITE_NOMEM;
+    }
+  }
+  /* Add sentinel */
+  cipherParams[n] = params[n];
+  cipherParams[n].m_name = globalSentinelName;
+
+  ++globalCipherCount;
+  cipherName = globalCipherNameTable[globalCipherCount].m_name;
+  strcpy(cipherName, desc->m_name);
 
-    globalCodecDescriptorTable[globalCipherCount - 1] = *desc;
-    globalCodecDescriptorTable[globalCipherCount - 1].m_name = cipherName;
+  globalCodecDescriptorTable[globalCipherCount - 1] = *desc;
+  globalCodecDescriptorTable[globalCipherCount - 1].m_name = cipherName;
 
-    globalCodecParameterTable[globalCipherCount].m_name = cipherName;
-    globalCodecParameterTable[globalCipherCount].m_id = globalCipherCount;
-    globalCodecParameterTable[globalCipherCount].m_params = cipherParams;
+  globalCodecParameterTable[globalCipherCount].m_name = cipherName;
+  globalCodecParameterTable[globalCipherCount].m_id = globalCipherCount;
+  globalCodecParameterTable[globalCipherCount].m_params = cipherParams;
 
-    /* Copy parameters */
-    for (n = 0; n < np; ++n)
+  /* Make cipher default, if requested */
+  if (makeDefault)
+  {
+    CipherParams* param = globalCodecParameterTable[0].m_params;
+    for (; param->m_name[0] != 0; ++param)
     {
-      char* paramName = (char*) sqlite3_malloc((int)strlen(params[n].m_name) + 1);
-      strcpy(paramName, params[n].m_name);
-      cipherParams[n] = params[n];
-      cipherParams[n].m_name = paramName;
+      if (sqlite3_stricmp("cipher", param->m_name) == 0) break;
     }
-    /* Add sentinel */
-    cipherParams[n] = params[n];
-    cipherParams[n].m_name = globalSentinelName;
-
-    /* Make cipher default, if requested */
-    if (makeDefault)
+    if (param->m_name[0] != 0)
     {
-      CipherParams* param = globalCodecParameterTable[0].m_params;
-      for (; param->m_name[0] != 0; ++param)
-      {
-        if (sqlite3_stricmp("cipher", param->m_name) == 0) break;
-      }
-      if (param->m_name[0] != 0)
-      {
-        param->m_value = param->m_default = globalCipherCount;
-      }
+      param->m_value = param->m_default = globalCipherCount;
     }
-
-    rc = SQLITE_OK;
-  }
-  else
-  {
-    rc = SQLITE_NOMEM;
   }
 
-  return rc;
+  return SQLITE_OK;
 }
 
 SQLITE_API int
@@ -682,13 +688,7 @@ sqlite3mcTermCipherTables()
   {
     if (globalCodecParameterTable[n].m_name[0] != 0)
     {
-      int k;
-      CipherParams* params = globalCodecParameterTable[n].m_params;
-      for (k = 0; params[k].m_name[0] != 0; ++k)
-      {
-        sqlite3_free((char*) params[k].m_name);
-      }
-      sqlite3_free(globalCodecParameterTable[n].m_params);
+      mcFreeCipherParams(globalCodecParameterTable[n].m_params);
     }
   }
   globalCipherCount = 0;
@@ -755,6 +755,11 @@ sqlite3mc_initialize(const char* arg)
       rc = sqlite3mc_vfs_create(NULL, 1);
     }
   }
+  if (rc != SQLITE_OK)
+  {
+    /* SQLite does not call sqlite3mc_shutdown after a failed initialization */
+    sqlite3mcTermCipherTables();
+  }
   return rc;
 }