Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix out-of-bounds access in test #105

Closed
wants to merge 2 commits into from
Closed

Fix out-of-bounds access in test #105

wants to merge 2 commits into from

Conversation

jkrems
Copy link
Contributor

@jkrems jkrems commented Sep 30, 2022

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
Copy link
Owner

aklomp commented Oct 10, 2022

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
Copy link
Contributor Author

jkrems commented Oct 13, 2022

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
Copy link
Owner

aklomp commented Oct 13, 2022

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 pushed a commit that referenced this pull request Oct 13, 2022
@aklomp
Copy link
Owner

aklomp commented Oct 13, 2022

Merged after rebasing.

@aklomp aklomp closed this Oct 13, 2022
aklomp pushed a commit that referenced this pull request Oct 13, 2022
Found with `-fsanitize=bounds`.

Resolves #105.
@aklomp aklomp reopened this Oct 13, 2022
@aklomp aklomp closed this in cba709a Oct 13, 2022
@aklomp
Copy link
Owner

aklomp commented Oct 13, 2022

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...

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.

2 participants