fix soundness problems in String and StringBuilder #218
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@MarcusGrass in case you want to check it out. :) |
MarcusGrass
reviewed
Feb 23, 2025
noib3
reviewed
Mar 16, 2025
Comment on lines
+246
to
+264
| pub fn finish(mut self) -> String { | ||
| // when constructing the final string, the pointer it contains must always be non null and | ||
| // terminated with a null byte | ||
| if self.inner.data.is_null() { | ||
| // ensure we are in a valid state | ||
| assert!(self.inner.is_empty()); | ||
| assert_eq!(self.cap, 0); | ||
|
|
||
| // The pointer of `String` should never be null, and it must be | ||
| // terminated by a null byte. | ||
| if s.data.is_null() { | ||
| unsafe { | ||
| let ptr = libc::malloc(1) as *mut i8; | ||
| if ptr.is_null() { | ||
| unable_to_alloc_memory(); | ||
| } | ||
| ptr.write(0); | ||
|
|
||
| s.data = ptr; | ||
| } | ||
| } | ||
| // Create a null terminated empty string | ||
| self.inner = String::from_bytes(&[]); | ||
| } else { | ||
| debug_assert!(self.cap > self.inner.len()); | ||
| assert!(self.cap > self.inner.len()); | ||
| } | ||
|
|
||
| assert_eq!( | ||
| unsafe { *self.inner.data.add(self.inner.len()) }, | ||
| 0, | ||
| "StringBuilder should always return a null terminated string" | ||
| ); |
There was a problem hiding this comment.
These are all just sanity checks, right? If so, debug_assertions should be enough.
There was a problem hiding this comment.
The assertions in the null check is just a sanity check so a debug_assert is fine there and can change it if its preferred.
The other assertions involve soundness so the assertion failing can result in UB.
For example assert!(self.cap > self.inner.len()); failing can result in UB as we might be passing bytes that are not initialized.
As for the other assertion, not being null terminated can cause UB depending on how the string is cloned inside neovim.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There were a few methods where a null pointer was used or the string was not null terminated.
This fixes possible undefined behaviors. To avoid these issues in the future a few assertions were added into
StringBuilder::finishto ensure that aStringis never initialized with a null pointer and is always terminated with a null byte.