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 null-termination bug w/ string->int cast #21146

Merged

Conversation

jeremiah-corrado
Copy link
Contributor

@jeremiah-corrado jeremiah-corrado commented Dec 2, 2022

During AoC activities, we identified a bug involving a missing null-terminator in strings created by IO.readLine. The bug caused leftover bytes from previous readLine operations to show up in casts from string to int.

This PR modifies IO.readStringBytesData to properly terminate strings modified by readLine(ref s: string) and readLine(ref b: bytes)

It also ensures that string buffers are properly terminated before casts to integral types, by modifying _cleanupForNumericCast.

An additional test from AoC activities was added to confirm the validity of the fix.

  • passing paratest

… failure mode.

Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
@mppf
Copy link
Member

mppf commented Dec 2, 2022

During AoC activities, we identified a bug involving a missing null-terminator in strings created by IO.readLine.

I was expecting that this PR would adjust IO.readLine to add a null terminator there somewhere but it does not. Why not?

@jeremiah-corrado
Copy link
Contributor Author

jeremiah-corrado commented Dec 2, 2022

Offline, @vasslitvinov suggested modifying _cleanupForNumericCast and it appears to have worked, so I decided to go with that.

I suppose adding a null terminator to readLine would be more correct tho? Maybe to protect against similar bugs on other casts?

Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
@@ -1426,6 +1426,9 @@ module BytesStringCommon {
}
}
}

// ensure that there is a null byte at the end of the buffer
x.buff[x.buffLen] = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend if x.buffLen > 0 here around this, unless you know that x cannot be the empty string at this point.

Signed-off-by: Jeremiah Corrado <jeremiah.corrado@hpe.com>
@jeremiah-corrado jeremiah-corrado merged commit 251e44c into chapel-lang:main Dec 2, 2022
@bradcray
Copy link
Member

bradcray commented Dec 2, 2022

Thanks so much for picking this up quickly @jeremiah-corrado! Wish we'd discovered it prior to AoC 2022!

@jeremiah-corrado
Copy link
Contributor Author

Sure thing, at least we got it early!

@bradcray
Copy link
Member

bradcray commented Dec 2, 2022

True! What if it hadn't been until day 10! :O

@jeremiah-corrado jeremiah-corrado deleted the readLine-bug-fix branch March 29, 2023 18:00
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.

3 participants