Handle allocation failures in cipher registration and parameter table cloning

[LucaCappelletti94]

Description

This PR handles allocation failures during sqlite3_initialize and sqlite3_open without NULL-pointer writes or SQL callbacks referencing freed cipher parameters. Failed initialization releases its partially registered ciphers so initialization can be retried. Cloned parameter tables share one allocation, owned by the connection and released through sqlite3_free.

Problem Statement

On main at 7a7f16a, sqlite3mcRegisterCipher passes an unchecked allocation to strcpy, and sqlite3mcCloneCodecParameterTable checks only one of its two allocations before writing through both. A single failed allocation can crash initialization or connection opening.

mcRegisterCodecExtensions also ignores sqlite3_set_clientdata's return value. That function invokes its supplied destructor on allocation failure, leaving SQL callbacks bound to freed parameters if registration continues. A failed initialization can also leave globalCipherCount inconsistent with the tables, causing a retry to return SQLITE_OK with an unusable cipher configuration.

• Reproducible test case included
• Compiler / sanitizer evidence provided
• Verified incorrect behavior shown in code
• Reference to external technical source (if applicable)

SQLite documents graceful allocation failure handling, allocation fault injection, and client-data destruction on allocation failure.

Verification

• Locally reproducible
• Unit test added/updated
• Static analysis / sanitizer output
• Other (please specify)

The included allocation-failure test fails each allocation in turn. Calls must return SQLITE_OK or SQLITE_NOMEM, retried initialization must reproduce the clean-start cipher tables, and successfully opened connections must retain their cipher configuration. Run it with

autoreconf
./configure
make oomtest
./oomtest

Compiled against the baseline sources with -fsanitize=address,undefined, the test reports

src/sqlite3mc.c:594:7: runtime error: null pointer passed as argument 1, which is declared to never be null
    #0 in sqlite3mcRegisterCipher src/sqlite3mc.c:594
    #1 in sqlite3mc_initialize src/sqlite3mc.c:704
    #2 in sqlite3_initialize src/sqlite3patched.c:187731
ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000

The configured build passes 40 initialization and 214 connection-opening failure points. The direct sanitizer build passes 40 and 34 respectively, with no AddressSanitizer, UndefinedBehaviorSanitizer, or leak reports. Allocation counts depend on enabled features.

Type of Change

• Bug Fix (requires verifiable evidence)
• Feature / Enhancement (new functionality)
• Refactor (no behavior change)
• Documentation
• Other (please describe)

Problem / Motivation

A required allocation failure should return SQLITE_NOMEM without terminating the process or publishing incomplete cipher state. The regression test exercises this contract and initialization retries in the Unix CI job.

Checklist

• I have independently verified (and suffered) the issue
• I am not submitting unverified or speculative changes
• I understand that AI-assisted changes must be reviewed by a human before submission

[utelle]

IMHO it would have been much better to start with reporting an issue instead of submitting this PR. That would have allowed to discuss what is actually an issue and what not, and how to solve it.

I agree that there are (mainly theoretical) memory allocation issues, and checks should be introduced to guard against such events.

This PR handles allocation failures during sqlite3_initialize and sqlite3_open without NULL-pointer writes or SQL callbacks referencing freed cipher parameters. Failed initialization releases its partially registered ciphers so initialization can be retried. Cloned parameter tables share one allocation, owned by the connection and released through sqlite3_free.

I prefer to keep separate allocations. It makes the code more readable, and reduces the risk of miscalculating the memory requirements in case of changes.

On main at 7a7f16a,

This link doesn't point to anything useful, but I think I understand what you mean.

sqlite3mcRegisterCipher passes an unchecked allocation to strcpy,

Function mcCheckValidName() is called for all name strings that are copied via strcpy(), ensuring that the names are valid (not NULL, not empty, not longer than allowed). However, you are right that on line 593 of sqlite3mc.c memory is allocated for name strings, but not checked whether the allocation succeeded. This has to be addressed, of course.

and sqlite3mcCloneCodecParameterTable checks only one of its two allocations before writing through both.

If the first allocation fails, the second will typically fail as well, and the second one is checked. However, I agree that both should be checked to be on the safe side.

mcRegisterCodecExtensions also ignores sqlite3_set_clientdata's return value. That function invokes its supplied destructor on allocation failure, leaving SQL callbacks bound to freed parameters if registration continues.

You are right again. However, if the system is that short of memory, you are unlikely to be able to work with SQLite anyway. No excuse, but unlikely to cause crashes in real world applications. Nevertheless, the issue will be addressed, of course.

[LucaCappelletti94]

A-Ok on issue first, and yes these issues only matter to very memory constrained settings (I am hitting them in a fuzzer where I am using a custom allocator designed to misbehave and find these things). These are arguably NOT urgent errors, just stuff I have collected as I am increasing the tests in the two *-src crates. I understand that what I am doing is a form of extreme stress testing that has little to do with reality, but it can yield important bugs on occasion.

I suppose in real life one could hit them in very constrained embedded systems, and even there it is pushing it.
Fixing these bugs has the (arguably circular) benefit that the fuzzer won't any longer find them and potentially find other things of stronger interest.

Sorry for the (functionally) dead link, I meant to link the relevant lines but I must have ended up copying the wrong thing at some point and I ended up not seeing it at the time.

[utelle]

A-Ok on issue first, and yes these issues only matter to very memory constrained settings (I am hitting them in a fuzzer where I am using a custom allocator designed to misbehave and find these things).

Of course, it is important to catch even unlikely issues. Therefore thank you for reporting your findings.

These are arguably NOT urgent errors, just stuff I have collected as I am increasing the tests in the two *-src crates. I understand that what I am doing is a form of extreme stress testing that has little to do with reality, but it can yield important bugs on occasion.

Sure. And as said I will address them.

[utelle]

Commit 95b6a40 addresses the issues you detected. Thanks again for reporting.

I applied fixes somewhat differently than you did in your PR, and added a few extra measures to make your oomtest run successfully. And, I added your oomtest.c application to the CI workflow. Thanks for providing the code.

The oomtest runs without reporting any failures. Therefore I'm closing the PR...

[LucaCappelletti94]

Thanks! I appreciate it!