Fix JS_WriteObject2 to check all malloc failures #1191
Conversation
Add dbuf_error() check at the end of JS_WriteObject2() to detect buffer allocation failures during BJSON encoding. Without this check, if malloc/realloc fails at certain points during encoding, the error flag is set in the DynBuf but never checked. JS_WriteObject2() returns the incomplete buffer as if encoding succeeded, causing data corruption. Here we ensure that when dbuf.error is set, an OutOfMemory exception is thrown and the function returns NULL. Co-Authored-By: Claude <[email protected]>
| goto fail; | ||
| if (JS_WriteObjectAtoms(s)) | ||
| goto fail; | ||
| if (dbuf_error(&s->dbuf)) { |
There was a problem hiding this comment.
I suspect you'll need at least this additional change to get past asan/msan/etc., but it's quite possible more work is needed:
diff --git a/quickjs.c b/quickjs.c
index 793a13e..b5effe4 100644
--- a/quickjs.c
+++ b/quickjs.c
@@ -35793,6 +35793,8 @@ static int JS_WriteObjectAtoms(BCWriterState *s)
DynBuf dbuf1;
int i, atoms_size;
+ if (dbuf_error(&s->dbuf))
+ return -1;
dbuf1 = s->dbuf;
js_dbuf_init(s->ctx, &s->dbuf);
bc_put_u8(s, BC_VERSION);Note how JS_WriteObjectAtoms moves s->dbuf to a local variable, then reinitializes it. It does that so it can stuff in the atoms first.
There was a problem hiding this comment.
Is there a reason asan/msan/etc. isn't enabled for building the tests?
There was a problem hiding this comment.
You mean locally? You can turn it on by passing -DQJS_ENABLE_ASAN, -DQJS_ENABLE_MSAN, etc. to cmake.
There was a problem hiding this comment.
Thanks, but I meant in the Makefile, i.e. why are the tests not always run with these flags, whether locally or in CI? Sorry if I'm missing something obvious, but that seems like it would be a good idea, as it would have saved you this part of the review 😊
There was a problem hiding this comment.
They are run on CI, of course; the test failures are on the asan/msan/etc. buildbots.
We can't assume sanitizers are locally available. As well, not all sanitizers compose, that's why CI tests them in isolation.
There was a problem hiding this comment.
Oh, of course. Sorry, doing too many things at once this morning. I'll take a look in a short while.
| for (size_t limit_kb = 10; limit_kb <= 200; limit_kb += 1) { | ||
| JSRuntime *rt = JS_NewRuntime(); | ||
| JSContext *ctx = JS_NewContext(rt); | ||
|
|
There was a problem hiding this comment.
Minor style nit but the house style for code inside functions is to leave out blank lines pretty much all the time.
2 participants
Add
dbuf_error()check at the end ofJS_WriteObject2()to detect buffer allocation failures during BJSON encoding.Without this check, if malloc/realloc fails at certain points during encoding, the error flag is set in the
DynBufbut never checked.JS_WriteObject2()returns the incomplete buffer as if encoding succeeded, causing data corruption.Here we ensure that when
dbuf.erroris set, anOutOfMemoryexception is thrown and the function returns NULL.