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 potential buffer overrun when node_name calls stoupper #1108

Conversation

jsberg-bnl
Copy link

name is a Fortran string and is not null terminated.
stoupper searches for the null termination.
Replace stoupper call by running toupper over the length of the Fortran string.

name is a Fortran string and is not null terminated.
stoupper searches for the null termination.
Replace stoupper call by running toupper over the length of the Fortran string.
@rdemaria
Copy link
Contributor

Thanks! Do you have a case in which the buffer overrun happens?

@jsberg-bnl
Copy link
Author

jsberg-bnl commented May 21, 2022

I do, though I haven't chased down what specifically triggered it. The sanitizer caught it when I was chasing down a different bug. I can upload a zip file on Monday if you like, though it is a rather large lattice.

@rdemaria
Copy link
Contributor

Don't bother. It was just to understand if there were other traps like this lurking around.

@rdemaria rdemaria merged commit 5c92732 into MethodicalAcceleratorDesign:master May 22, 2022
@jsberg-bnl
Copy link
Author

I would definitely be on the lookout for things like this. The C/Fortran mixing as done here is not particularly robust, in particular when it comes to string handling (e.g., you'll notice that a lot of calls with string literals have an extra space at the end of the string; that is there to address this sort of issue, and you just need to know "when you make this call, append an extra space."). I didn't do a complete review of all the stoupper calls, but I glanced at a couple of other occurrences, and they were using C strings and so should be fine, but it would probably be worth checking them all. Running the test suite with the sanitize flags (and maybe GC off) might be enlightening.

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.

None yet

2 participants