Skip to content

Initialise padding after the file buffer #249

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

Merged
merged 1 commit into from
Mar 27, 2025
Merged

Conversation

wcawijngaards
Copy link
Member

The change initializes the padding after the file buffer. Otherwise checkers report use of uninitialised memory from those bytes. It zeroes the block size after the fread, so it does not have to wipe the entire buffer. For strings, that means the library caller would have to initialize them.

@wcawijngaards wcawijngaards self-assigned this Mar 7, 2025
@lemire
Copy link
Collaborator

lemire commented Mar 10, 2025

Alternatively, you can silence the sanitizer warnings.

@wcawijngaards
Copy link
Member Author

That is true, tools have options for that. I have not used those before. I think the zero of the padding is the best choice, it is not expensive because only padding per file read, of a couple bytes per 16Kb read, and also provides actual initialization, in a defense in depth sense a good measure. If it was expensive, it can get a configure option for maintenance mode and clean debug output.

@wcawijngaards wcawijngaards merged commit 8e8b396 into main Mar 27, 2025
11 checks passed
wcawijngaards added a commit that referenced this pull request Mar 27, 2025
- Initialise padding after the file buffer (#249).
- Fix type NSAP-PTR (#250).
- Fix LOC poweroften lookup (#251).
@wcawijngaards
Copy link
Member Author

I did a number of speed tests with and without the padding initialization, but I could not get a reading for a speed difference. The speed change is likely very small, less than a percent, and I could not capture it with bunch of Mbs and about a Gb zone file. So I think the speed concern need not be an issue.

@wtoorop
Copy link
Member

wtoorop commented Jun 5, 2025

@wcawijngaards how did you see those read from uninitialized value errors? I'm trying to reproduce, but cannot detect them with valgrind -s and also not when compiling with -fsanitize=address,undefined,leak -fno-sanitize-recover=all

@wcawijngaards
Copy link
Member Author

By running the tests from tpkg. Some routines failed on them. Perhaps dname, testplan_axfr, or unknown_rr or so. I believe it failed for both those debug settings.

@wtoorop
Copy link
Member

wtoorop commented Jun 5, 2025

By running the tests from tpkg. Some routines failed on them. Perhaps dname, testplan_axfr, or unknown_rr or so. I believe it failed for both those debug settings.

Thanks. I'll give running the tpkg tests with -fsanitize=address,undefined,leak -fno-sanitize-recover=all a try then.

@wcawijngaards
Copy link
Member Author

Just a note, this fix is already part of the simdzone version that is in the NSD code repository, and 4.12.0 too. So it does not show them.

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