-
Notifications
You must be signed in to change notification settings - Fork 347
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set SQLITE_LIMIT_FUNCTION_ARG to SQLite's normal default. #2493
Conversation
People keep legitimately running up against this limit. It's not really clear what value it is providing, so let's just match the default. Fixes #2412
Just to add color here, according to https://www.sqlite.org/limits.html (search for "Maximum Number Of Arguments On A Function"), the default is 100 and absolute maximum is 127 because this value is sometimes stored in an unsigned char. Sure enough, sqliteLimit.h has:
I can't seem to figure out where the default of 100 is set. |
@@ -809,7 +809,9 @@ void SqliteDatabase::setupSecurity() { | |||
// https://www.sqlite.org/limits.html#max_compound_select | |||
sqlite3_limit(db, SQLITE_LIMIT_COMPOUND_SELECT, 5); | |||
sqlite3_limit(db, SQLITE_LIMIT_VDBE_OP, 25000); | |||
sqlite3_limit(db, SQLITE_LIMIT_FUNCTION_ARG, 32); | |||
// For SQLITE_LIMIT_FUNCTION_ARG we use the default instead of the "security" recommendation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/default/maximum/ ?
Weird, https://www.sqlite.org/security.html says that the default is 127, not 100. My intent was to use the default. I guess I can try to find out what sqlite actually reports its own default as being... |
Digging more: the default is 127 and https://www.sqlite.org/limits.html is buggy. Here's what my
The limits from the compile-time constants are initialized into a
So it's the default and maximum. |
sqlite3_limit(db, SQLITE_LIMIT_FUNCTION_ARG, 32); | ||
// For SQLITE_LIMIT_FUNCTION_ARG we use the default instead of the "security" recommendation | ||
// because there are too many valid use cases for large argument lists, especially json_object. | ||
sqlite3_limit(db, SQLITE_LIMIT_FUNCTION_ARG, 127); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this might change in the future, maybe we should add a link to the existing documentation?
PS: https://www.sqlite.org/c3ref/create_function.html Also mentions max 127 value. |
OK seems like this is correct as-is, just gonna merge. |
@kentonv Hi, when is this gonna be released up stream into production ? Unfortunately, i am hitting the upper limit of 32 (I have 36 params in json_object) |
People keep legitimately running up against this limit. It's not really clear what value it is providing, so let's just match the default.
Fixes #2412