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

Address sanitizer issue in private_locateSignature function (NOZUnzipper.m) #46

Closed
ryuzmukhametov opened this issue Jan 30, 2017 · 4 comments

Comments

@ryuzmukhametov
Copy link

Hi guys,

Just tested my code (the code uses ZipUtilities 1.9.0) using address sanitizer checkbox and find out a read access to wrong address.
Please see attached screenshot to get additional details.
The problem is next: the function uses array with length=pageSize, and it reads array element by buffer[pageSize] in for-loop below. I see it's just a read operations and it doesn't have a big impact, but I'm curious, could it be a reason for crash in production code without address sanitizer option?
details
.

@NSProgrammer
Copy link
Owner

Can you indicate what file this is happening in? Also, can you run the same test on the latest in master to see if it reproduce with the freshest code?

@ryuzmukhametov
Copy link
Author

The issue is preserved in latest master. And it is risen only with AddressSanitiser checkbox (without it everything is ok).
The key lines are in file NOZUnzipper.m:
444: size_t sizeToRead = sizeof(buffer);
461: for (off_t i = (off_t)(sizeToRead - 3); i >= 0; i--) {
462: if (buffer[i + 3] == sig[3]) {
So the code tries to read buffer[sizeof(buffer)] in last line. As result it leads to out of range error.
The possible solution is starting for-loop from i=(off_t)(sizeToRead - 2). But I am not sure about possible side effects.

@NSProgrammer
Copy link
Owner

it was an off by 1 error (like you pointed out) but I needed "sizeToRead - 4" instead of "sizeToRead - 3". "sizeToRead - 2" would have just hit the same issue twice :)

Fixed

@ryuzmukhametov
Copy link
Author

Sure, "sizeToRead - 4" is correct, my mistake :)

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

No branches or pull requests

2 participants