Skip to content

fix size overflow in S_strbuf_grow_by and S_parser_feed - #654

Open
zayeem06 wants to merge 2 commits into
commonmark:masterfrom
zayeem06:strbuf-size-overflow
Open

zayeem06 wants to merge 2 commits into
commonmark:masterfrom
zayeem06:strbuf-size-overflow

Conversation

@zayeem06

@zayeem06 zayeem06 commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

S_strbuf_grow_by adds buf->size and add as int32_t, so a chunk near 2 GiB wraps the target size negative, cmark_strbuf_grow returns without growing (its assert is compiled out in Release) and cmark_strbuf_put writes past the buffer. cmark_markdown_to_html hits this on a, a NUL byte and 0x7fffffff bytes with no newline, which ASan reports as a heap-buffer-overflow write of 2147483647 bytes, and a single line of 2 GiB or more makes chunk_len negative in S_parser_feed, which moves buffer backwards and reads out of bounds. Both now take the abort cmark_strbuf_grow already uses for oversized buffers, so please check that aborting is the behaviour you want here; there is no test because the trigger needs a 2 GiB buffer and ends in abort().

buf->size + add is computed as int32_t, so a chunk near 2 GiB wraps to
a negative target size, cmark_strbuf_grow returns without growing and
cmark_strbuf_put writes past the buffer. A line of 2 GiB or more also
truncates chunk_len in S_parser_feed, which then moves buffer backwards
and reads out of bounds. Take the existing abort path for both.
@jgm

jgm commented Nov 2, 2026

Copy link
Copy Markdown
Member

@nwellnhof can you have a look?

@nwellnhof

Copy link
Copy Markdown
Contributor

Maybe it's better to make the strbuf functions take a size_t or ptrdiff_t length? The buffer size would still be checked against INT32_MAX, but this would make code involving pointer arithmetic more safe, for example allowing cmark_strbuf_put(buf, data, end - data).

To find such issues, the clang option -Wshorten-64-to-32 is a great help. Interestingly, it only spots the code in question:

src/blocks.c:618:22: warning: implicit conversion loses integer precision: 'long' to 'bufsize_t' (aka 'int') [-Wshorten-64-to-32]
  618 |     chunk_len = (eol - buffer);
      |               ~  ~~~~^~~~~~~~
1 warning generated.

@jgm

jgm commented Nov 6, 2026

Copy link
Copy Markdown
Member

I'd have no objections to that.
And maybe we could add that flag to the build flags for clang?

cmark_strbuf_grow, cmark_strbuf_set and cmark_strbuf_put now take a
ptrdiff_t length, so pointer differences can be passed without
truncation. The buffer size is still limited by the check in
cmark_strbuf_grow. S_parser_feed, S_process_line and cmark_utf8proc_check
carry the line length as ptrdiff_t too, which makes the separate
long-line abort in S_parser_feed unnecessary.

Also build with -Wshorten-64-to-32 on clang.
@zayeem06

zayeem06 commented Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Switched cmark_strbuf_grow/set/put to ptrdiff_t lengths. I went with ptrdiff_t over size_t so the existing len <= 0 early return still covers callers that compute a negative length. S_parser_feed, S_process_line and cmark_utf8proc_check now carry the line length as ptrdiff_t, so the extra long-line abort in S_parser_feed is gone and an oversized line hits the INT32_MAX / 2 check in cmark_strbuf_grow like everything else.

I also added -Wshorten-64-to-32 for Clang/AppleClang in cmark_add_compile_options. The clang build is warning-free with it, and the tests pass. The three 2 GiB cases from the description (NUL in the line, two feeds, one long line) all abort in cmark_strbuf_grow under ASan instead of overflowing.

The only behaviour change worth a look is in utf8proc_valid, which now gets a ptrdiff_t length. It only compares that length against the 1-4 byte sequence length, so the cast back to int on the short-input path is safe.

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.

3 participants