Skip to content

fix(cutils): handle vsnprintf encoding error in dbuf_printf#1482

Open
andreasrosdal wants to merge 1 commit into
quickjs-ng:masterfrom
nordstjernen-web:claude/sec-dbuf-vsnprintf-error
Open

fix(cutils): handle vsnprintf encoding error in dbuf_printf#1482
andreasrosdal wants to merge 1 commit into
quickjs-ng:masterfrom
nordstjernen-web:claude/sec-dbuf-vsnprintf-error

Conversation

@andreasrosdal
Copy link
Copy Markdown
Contributor

When vsnprintf returns -1 (encoding error - e.g. an %ls conversion that hits an invalid multibyte sequence), dbuf_printf's slow path advanced s->size by len = -1, wrapping the size_t to ~SIZE_MAX. Subsequent dbuf operations then treated the buffer as essentially unlimited and indexed off the end.

Bail out, flag the buffer as errored, and let callers propagate.

No automated test: dbuf_printf is internal and reaching the vsnprintf encoding-error branch requires either an invalid multibyte input to %ls (locale-dependent and not portable) or a vsnprintf implementation that returns -1 for other reasons. Reviewed by inspection.

When vsnprintf returns -1 (encoding error - e.g. an %ls conversion that
hits an invalid multibyte sequence), dbuf_printf's slow path advanced
s->size by len = -1, wrapping the size_t to ~SIZE_MAX. Subsequent
dbuf operations then treated the buffer as essentially unlimited and
indexed off the end.

Bail out, flag the buffer as errored, and let callers propagate.

No automated test: dbuf_printf is internal and reaching the vsnprintf
encoding-error branch requires either an invalid multibyte input to
%ls (locale-dependent and not portable) or a vsnprintf implementation
that returns -1 for other reasons. Reviewed by inspection.
Comment thread cutils.h
Comment on lines +849 to +854
if (len < 0) {
/* vsnprintf encoding error: don't let the caller wrap s->size by
advancing it by -1, which would underflow to near SIZE_MAX. */
s->error = true;
return -1;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should just be an assert? I'm trying to think of a scenario where len < 0 could happen that is not a bug on our side, but I can't come up with anything.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, this is not exposed to user code to it must be an internal one, so let's assert on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants