aklomp / base64

Fast Base64 stream encoder/decoder in C99, with SIMD acceleration
BSD 2-Clause "Simplified" License
890 stars 165 forks source link

Fix out-of-bounds access in test #105

Closed jkrems closed 2 years ago

jkrems commented 2 years ago

When running the tests with address sanitizer enabled, it fails with the following error:

test/test_base64.c:198:40: runtime error: index 402 out of bounds for type 'char[400]'
    #0 0x7f322c00c749 in test_streaming test/test_base64.c:198:40
    #1 0x7f322c00c749 in test_one_codec test/test_base64.c:340:10
    #2 0x7f322c00c749 in main test/test_base64.c:361:11

I think adding this bounds check preserves the semantics of the test but I'm not super familiar with the codebase.

aklomp commented 2 years ago

Thanks for the PR. Looks like a real issue, an out-of-bounds pointer is being dereferenced.

Your fix works, and will bail out the loop before the dereference happens. The following code might make the intent a bit clearer:

while (base64_stream_decode(&state, &ref[inpos], (inpos + bs > reflen) ? reflen - inpos : bs, &enc[enclen], &partlen)) {
     enclen += partlen;
     inpos += bs;

    // Has the entire buffer been consumed?
    if (inpos >= 400) {
        break;
    }
}

But no need to change it.

As a side note, I'd be interested in running asan in CI. Did you find this issue with gcc or clang's address sanitizer, or were you using another tool?

jkrems commented 2 years ago

Updated to use the suggested pattern.

More precisely, this was found via clang's -fsanitize=bounds. Needed to look up the exact setup because I was building via bazel. The error will show up in this projects setup with:

make -C test CFLAGS=-fsanitize=bounds

That is, it will print an error though exit code is still 0. I didn't dig more deeply than that. :)

aklomp commented 2 years ago

Thanks for the update, I'll merge it shortly. Also thanks for posting the code to reproduce the warning. I've added it to my own build script.

aklomp commented 2 years ago

Merged after rebasing.

aklomp commented 2 years ago

Sorry for reopening and reclosing, I forgot to annotate the commit with Resolves #105. Made some stealthy force-pushes to master to fix. Nothing to see here...