Result of query with comment is inconsistent and random #975
Comments
kkrypt0nn
changed the title
|
Can you provide a self-contained reproduction? Preferably a single Alternatively, since you've seem to have narrowed down the possible cause, maybe you can contribute a failing test? But from what I understand you're saying it's not behaving consistently? |
|
FWIW, what you're doing is not optimal. It allows SQL injections (including injecting comments) and prevents you from re-using prepared statements. I would do sth. like |
What exactly do you mean with "self-contained reproduction"? I could provide a NestJS example which has the issue contained in it if needed.
Yep I am aware of that, the purpose of the application is to showcase it, hence why the tested input is actually an SQLi and why I am using the |
import Database from 'better-sqlite3';
const db = new Database(':memory:');
// Your code here.
Ok, so the problem is not that you're getting "The supplied SQL string contains more than one statement" errors but that you claim for the same input better-sqlite3 sometimes errors and sometimes doesn't? Correct? |
Absolutely, when spamming the same query over and over it leads to a previously failed query returning the correct data - which is there the big inconsistency. I will provide the reproduction as soon as possible 👍 |
I'm super curious to see. Right now I'm assuming the error is somewhere else in the chain and not in better-sqlite3 itself. But let's find out.
This could very easily be a race condition (or other type of bug, caching etc.) in your server or client code, making it look like that when in reality it is actually consistent. Hence why we need a repro without any other dependency. |
|
|
|
Seems like the issue is actually with the comments parsing here after all. When using template strings and new lines like in the example below, it will fail - though when using template strings and putting everything in a single line it succeeds perfectly. There is no reason it should fail for multiple statements as the query executed is SELECT *
FROM user_satellites_satellite
FULL JOIN satellite ON user_satellites_satellite.satelliteId = satellite.id
WHERE user_satellites_satellite.userId = '630633ba-030c-4967-acd4-d790bf26cfda'
AND (satellite.name LIKE '%ezrms7w8tzmjb3s02iu6') UNION SELECT tbl_name, CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR) FROM sqlite_master; --%' OR satellite.description LIKE '%ezrms7w8tzmjb3s02iu6') UNION SELECT tbl_name, CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR) FROM sqlite_master; --%');Which has only one statement as the rest is commented out. Source code used: import Database from "better-sqlite3";
const db = new Database("db.sqlite");
let failed = 0;
let succeeded = 0;
for (let i = 0; i < 5000; i++) {
const random = Array.from(Array(20), () =>
Math.floor(Math.random() * 36).toString(36)
).join("");
const searchQuery = `${random}') UNION SELECT tbl_name, CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR) FROM sqlite_master; --`;
const query = `SELECT *
FROM user_satellites_satellite
FULL JOIN satellite ON user_satellites_satellite.satelliteId = satellite.id
WHERE user_satellites_satellite.userId = '630633ba-030c-4967-acd4-d790bf26cfda'
AND (satellite.name LIKE '%${searchQuery}%' OR satellite.description LIKE '%${searchQuery}%');`;
try {
await db.prepare(query).all();
succeeded++;
} catch (_) {
failed++;
}
}
console.log(failed, succeeded); // Will never be consistentThe interesting part is that sometimes it works, sometimes it doesn't. You can also edit the code so that failed queries are executed again, some will work after some new attempts made. Running the exact same code with const sqlite3 = require("sqlite3").verbose();
const db = new sqlite3.Database("db.sqlite");
async function main() {
let failed = 0;
let succeeded = 0;
for (let i = 0; i < 5000; i++) {
const random = Array.from(Array(20), () =>
Math.floor(Math.random() * 36).toString(36)
).join("");
const searchQuery = `${random}') UNION SELECT tbl_name, CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR) FROM sqlite_master; --`;
const query = `SELECT *
FROM user_satellites_satellite
FULL JOIN satellite ON user_satellites_satellite.satelliteId = satellite.id
WHERE user_satellites_satellite.userId = '630633ba-030c-4967-acd4-d790bf26cfda'
AND (satellite.name LIKE '%${searchQuery}%' OR satellite.description LIKE '%${searchQuery}%');`;
try {
await db.prepare(query).all();
succeeded++;
} catch (_) {
failed++;
}
}
console.log(failed, succeeded); // 0 5000
}
main();The database used is available here: https://file.io/GaG5ThL0YbHs |
|
I'm getting consistent |
|
Are you sure it's actually inconsistent or are you expecting the following to not produce an error? import Database from "better-sqlite3";
const db = new Database(":memory:");
db.prepare(`SELECT 1; --%test`); |
That is it output I am getting:
When making the query a one line query with template strings, without new lines or tabs, it works as it should:
It is very inconsistent, and the query executed should never produce an error (hence must always return
Using that basic Dockerfile FROM node:19.8.1
WORKDIR /app
COPY db.sqlite /app
COPY main.js /app
COPY package-lock.json /app
COPY package.json /app
RUN npm iand getting an interactive shell with docker run -it $(docker build -q .) bashgives me sometimes fully failed, sometimes fully succeeded results:
When logging the error in the docker: |
|
If you're interested - those are the results when using Locally on macOS: Within a docker: |
|
I'm completely fascinated by this bug. I've created the following repo, which uses a memory database and does not use any random values. It behaves the same for Node 16 and 18. It will seemingly randomly work and then not work. The interesting bit is that when one of the three Or I only got a single very rare mix of Code: import Database from 'better-sqlite3';
function doIt() {
const db = new Database(':memory:');
db.exec(`
CREATE TABLE "satellite" ("id" varchar PRIMARY KEY NOT NULL, "name" varchar(255) NOT NULL, "description" varchar(255) NOT NULL, CONSTRAINT "UQ_ebcb5f391c90d59b38c2adecb13" UNIQUE ("name"));
CREATE TABLE "user" ("id" varchar PRIMARY KEY NOT NULL, "username" varchar(255) NOT NULL, CONSTRAINT "UQ_78a916df40e02a9deb1c4b75edb" UNIQUE ("username"));
CREATE TABLE "user_satellites_satellite" ("userId" varchar NOT NULL, "satelliteId" varchar NOT NULL, CONSTRAINT "FK_058e916625b4d0e5d920c62f2bb" FOREIGN KEY ("userId") REFERENCES "user" ("id") ON DELETE CASCADE ON UPDATE CASCADE, CONSTRAINT "FK_f3762d755df5bbba15af5507822" FOREIGN KEY ("satelliteId") REFERENCES "satellite" ("id") ON DELETE CASCADE ON UPDATE CASCADE, PRIMARY KEY ("userId", "satelliteId"));
`);
const searchQuery = `xxxxxxxxxxx') UNION SELECT tbl_name, CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR) FROM sqlite_master; --`;
const query = `SELECT *
FROM user_satellites_satellite
FULL JOIN satellite ON user_satellites_satellite.satelliteId = satellite.id
WHERE user_satellites_satellite.userId = '630633ba-030c-4967-acd4-d790bf26cfda'
AND (satellite.name LIKE '%${searchQuery}%' OR satellite.description LIKE '%${searchQuery}%');`;
try {
db.prepare(query);
console.log('OK');
} catch (err) {
console.error(err);
}
db.close();
}
doIt();
doIt();
doIt();With the length of This is like better than Netflix. Invite your friends over 😄 @JoshuaWise is pretty good with this type of stuff, any idea WTF is happening? 👀 |
Fully agree - if it was consistent it could potentially make sense, but right now from what I've seen is that it's fully random and if you spam failed query lots of times, at some point it will eventually succeed:
Within the docker it's a bit different but the same sort of happens:
Code used import Database from 'better-sqlite3';
function doIt(i) {
const db = new Database(':memory:');
db.exec(`
CREATE TABLE "satellite" ("id" varchar PRIMARY KEY NOT NULL, "name" varchar(255) NOT NULL, "description" varchar(255) NOT NULL, CONSTRAINT "UQ_ebcb5f391c90d59b38c2adecb13" UNIQUE ("name"));
CREATE TABLE "user" ("id" varchar PRIMARY KEY NOT NULL, "username" varchar(255) NOT NULL, CONSTRAINT "UQ_78a916df40e02a9deb1c4b75edb" UNIQUE ("username"));
CREATE TABLE "user_satellites_satellite" ("userId" varchar NOT NULL, "satelliteId" varchar NOT NULL, CONSTRAINT "FK_058e916625b4d0e5d920c62f2bb" FOREIGN KEY ("userId") REFERENCES "user" ("id") ON DELETE CASCADE ON UPDATE CASCADE, CONSTRAINT "FK_f3762d755df5bbba15af5507822" FOREIGN KEY ("satelliteId") REFERENCES "satellite" ("id") ON DELETE CASCADE ON UPDATE CASCADE, PRIMARY KEY ("userId", "satelliteId"));
`);
const replace = "x".repeat(i);
const searchQuery = `${replace}') UNION SELECT tbl_name, CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR) FROM sqlite_master; --`;
const query = `SELECT *
FROM user_satellites_satellite
FULL JOIN satellite ON user_satellites_satellite.satelliteId = satellite.id
WHERE user_satellites_satellite.userId = '630633ba-030c-4967-acd4-d790bf26cfda'
AND (satellite.name LIKE '%${searchQuery}%' OR satellite.description LIKE '%${searchQuery}%');`;
try {
db.prepare(query);
} catch (err) {
console.error("FAIL, attempting 50 times the same query and hoping for success...");
for (let index = 1; index <= 50; index++) {
try {
db.prepare(query);
console.log(`Success after attempt #${index}`)
break
} catch {}
}
}
db.close();
}
for (let index = 0; index < 50; index++) {
doIt(index);
} |
|
FWIW ChatGPT is entirely useless telling me for the third time that I need to escape |
|
Hello, fellow developers! I happened upon this most fascinating of issue, which intrigued me so much I elected to abandon an hour of perfectly good sleep in favour of debugging the crap out of this. The first thing I did was to insert a bunch of ugly for (char c; (c = *tail); ++tail) {
+ printf("%c", c);
if (IS_SKIPPED(c)) continue;
if (c == '/' && tail[1] == '*') {
tail += 2;
+ printf("*");
for (char c; (c = *tail); ++tail) {
printf("%c", c);
if (c == '*' && tail[1] == '/') {
tail += 1;
break;
}
}
} else if (c == '-' && tail[1] == '-') {
tail += 2;
+ printf("-");
for (char c; (c = *tail); ++tail) {
printf("%c", c);
if (c == '\n') break;
}
} else {
sqlite3_finalize(handle);
+ printf("\n");
return ThrowRangeError("The supplied SQL string contains more than one statement");
}
}
+ printf("\n");And in my trusty terminal, I began to see something... intriguing. Something unexpected. A pattern of noise just past the last character emerges:
Is this an out-of-bounds read? Surely, for (char a; (a = *tail); ++tail) {
for (char b; (b = *tail); ++tail) {
whatever(c);
}
}When the condition The solution to this loopy confusion requires more thought than my 1am brain can produce. After some sleep, perhaps a pull request will present itself... |
|
It's nice to see the chosen one show up in times of need 🙏 |
Your explanation: 🧑🍳 🤌 💯 Thanks for digging into this! |
Uh oh!
There was an error while loading. Please reload this page.
There seem to be an inconsistent bug in the comments parsing method.
Having the following raw SQL query (making it a one liner doesn't change anything either):
with
searchQuerybeing (randomcan be anything):random') UNION SELECT CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), tbl_name, CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR), CAST(1 AS VARCHAR) FROM sqlite_master; --And executing it with
query()Will either return the correct and expected data or the error
Logging with NestJS shows the following query is getting executed (query logged being always the same):
The exact same query being executed multiple times over and over, e.g. by spamming, will make the query return correct data after some time, which makes the bug inconsistent:
bug.mp4
Switching the SQLite driver to the
sqlite- being installed withnpm install sqlite3- driver makes the query always work and return the expected result, hence is not related to TypeORM itself.The text was updated successfully, but these errors were encountered: