Sqlite option getter toctou 2 - #70
Draft
trivikr wants to merge 5 commits into
Draft
Conversation
Signed-off-by: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: nodejs#65698 Refs: nodejs#65697 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
prepare(), function(), aggregate(), deserialize(), applyChangeset() and backup() validated the connection, then read their options bag with Object::Get(). A property getter runs arbitrary JavaScript at that point, so a getter calling close() invalidates what was just checked. Five of the six then passed a null sqlite3* to SQLite and crashed; prepare() reported a spurious "out of memory". Re-check IsOpen() after option parsing, immediately before the SQLite call, keeping the early check so invalid calls still fail before any user code runs. IsOpen() is the only condition a getter can change: authorizer and callback depths are RAII-managed. createSession() already parsed options first, so it only gains the early check. deserialize() also latched the buffer length before reading options.dbName. A getter that shrank the backing store left the length too large; CopyContents() then handed the uninitialized remainder to SQLite, from where serialize() returned it to JavaScript. Check the CopyContents() result instead of discarding it. function() and aggregate() cast the callback's length property with As<Int32>() and no IsInt32() guard. length is configurable, so any type reached the cast and produced a silently wrong arity. Fixes: nodejs#65586 Signed-off-by: Trevor Burnham <trevorburnham@gmail.com>
Add the skipIfSQLiteMissing() guard. Builds configured with shared libraries omit node:sqlite, so requiring it unconditionally failed the x86_64-darwin and aarch64-darwin shared-library CI jobs. Signed-off-by: Trevor Burnham <trevorburnham@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Experiments when reviewing https://www.github.com/nodejs/node/pull/65595