Cleanup `StringBuilder` methods #211

airblast-dev · 2024-12-30T20:24:16Z

I haven't added the necessary tests yet so marking this as draft.

Many of StringBuilder's methods had logic that really should have been their own function with safe wrappers.
This PR moves some logic to their own methods and allows public calls to allocate space without pushing bytes.

This should also reduce the effect of the memory leaks from String as String::from_bytes will now only allocate the minimal amount needed. This shouldn't change performance in any call site as the capacity is not stored, meaning no length changes are possible once a String is constructed. This only helps to reduce the allocated size where arguments require a String and a user calls String::from_bytes.

As part of the cleanup a redundant null check was removed from StringBuilder::push_bytes as libc::realloc already handles the null case.

I have made the commit messages to make things are easy to follow.
Will mark this ready for review once I have added some tests.

…tringBuilder::with_capacity`

use `StringBuilder::reserve` to avoid code duplication in `StringBuilder::push_bytes`. The null check and call to malloc was removed as `libc::realloc` already handles the null case properly. The extra capacity check was removed as `StringBuilder::reserve` only allocates if needed.

The builder would allocate extra capacity even though it is not possible to change the length of the buffer after initialization. This makes it so only the space needed gets allocated.

`StringBuilder::reserve` and `StringBuilder::reserve_exact` already handle the needed space for the null byte.

airblast-dev · 2024-12-31T10:49:38Z

Fixed things locally, will push the latest version with new tests added and existing ones passing.

While mainly a fix for previous commits, this adds a few safety checks and optimizations. In cases of inserting even 0 bytes an allocation would be created with a single null byte. This makes it so an allocation is only made if bytes are actually being inserted. NULL checks have been added for realloc as it may return a NULL pointer if it is unable to allocate. Instead of UB it will now panic.

this should be reverted once a more sophisticated string mechanism in place until then this at least decreases the amount that is leaked

We already call `StringBuilder::reserve_exact` so we already know the allocation cannot be shrinked further. This avoids an extra trip to the allocator in `String::from_bytes`.

airblast-dev · 2024-12-31T13:26:07Z

Added a few safety checks to avoid UB in edge cases. Passes the existing tests and the new ones.

The refactor makes it so String either points to null with a length of 0, or points to an allocation with a length bigger than zero. The old version would allocate even when pushing 0 bytes.

Lastly, a String allocation is now shrunk to its minimal size when calling StringBuilder::finish (this is not done in StringBuilder::finish_unchecked for cases where extra capacity is known to not exist such as String::from_bytes).

Should be ready for a review :)

Edit: In case the LOC is too much I can split this into multiple parts, though I think it would be pretty annoying to deal with as most of the changes are connected.

avoids extra computation and checks

no UB, but caused allocations when cap would fit in remaining

noib3

Looks good after comments are addressed and tests pass. Thanks!

crates/types/src/string.rs

noib3 · 2025-01-01T09:16:27Z

Note that a couple of tests are segfaulting.

airblast-dev · 2025-01-01T09:22:29Z

Yeah, not really sure why. Weird as nothing seems wrong in the String tests. Looking into it.

airblast-dev · 2025-01-01T11:06:28Z

Looked into the neovim source. Down the call chains the string is copied with xstrdup which doesn't handle the null ptr causing segfaults.

Not sure if it was intentional but String::from_bytes always allocating even for zero bytes was avoiding this problem.
This also means even String::new should allocate in case it is passed somewhere and it is attempted to be copied with memcpy or xstrdup...

Many of the functions work without an issue but to be safe its likely best to always allocate. For example the failing function relies on the null byte while some of them check the length, in order to even find the null byte the null pointer gets read and causes a segfault or the null pointer is used in a memcpy with mostly the same outcome.

I originally though that it could be an issue, but String::new() returning a null ptr made me think this was fine.

Not really sure what to do from here. Should I refactor the code to always allocate? or just leave this PR for the time being?

noib3 · 2025-01-01T11:22:42Z

Having to allocate even for empty strings kinda sucks, but it's an upstream issue. I think, at least for the time being, we should just add a comment explaining why we allocate even when not necessary.

in order to even find the null byte the null pointer gets read

I'd have to look into it, but why does Neovim even need strings to be null-terminated when they have a len field?

airblast-dev · 2025-01-01T12:18:35Z

I'd have to look into it, but why does Neovim even need strings to be null-terminated when they have a len field?

Not really sure tbh. I will look into the neovim source code again if I'm misunderstanding something or if there is an underlying bug somewhere. Regardless I am pretty sure that null pointers are a no-no. In case you want to try it out, set the minimum argument to StringBuilder::with_capacity to 1 inside String::from_bytes and all of the tests will pass.

After reading the implementation of nvim_create_user_command I decided to test if :lua vim.api.nvim_create_user_command('B', '\0bc\0d', {}) would register properly. Nope, it just registers an empty string. Essentially this means that a neovim string can contain null bytes as much as it wants, but it doesn't mean it will work properly depending on the code path. This confirms that the null byte must always exist at the end of the string even if the pointer is null or the length is zero.

A few notes in case someone is trying to fix things:

String must always contain a null byte at its end regardless of if the len is 0 or the pointer is null.
String may contain a null byte inside of it, however depending on the code path the string may be cut off at the first null byte.
String may point to null in some functions (functions that do a length check and/or null check). But it may cause UB or segfaults in others.

noib3 · 2025-01-01T15:21:25Z

I'll do some more investigation to better understand strings' usage in Neovim.

As far as this PR is concerned, I say we just remove or comment out the if cap == 0 check in StringBuilder::with_capacity(), which should fix the segfaults.

not sure what changed

Even though we have a `max_width` of 79, rustfmt doesn't enforce it in comments. I do.

noib3 · 2025-01-02T15:11:31Z

Thanks!

airblast-dev added 8 commits December 30, 2024 22:50

implement StringBuilder::reserve, StringBuilder::reserve_exact `S…

4d555fb

…tringBuilder::with_capacity`

use StringBuilder::reserve_exact in String::from_bytes

259a22a

The builder would allocate extra capacity even though it is not possible to change the length of the buffer after initialization. This makes it so only the space needed gets allocated.

reuse StringBuilder::reserve_exact in StringBuilder::reserve

5aebd6d

add comment for StringBuilder::push_bytes

62ea409

Merge branch 'main' into string_cleanup

33561c0

avoid allocating for 0 capacity in StringBuilder::reserve*

beca9ba

dont allocate for an extra byte in StringBuilder::push_bytes

23f858d

`StringBuilder::reserve` and `StringBuilder::reserve_exact` already handle the needed space for the null byte.

airblast-dev added 9 commits December 31, 2024 15:14

add tests

8b6f8c8

remove _ casts to avoid type changes with refactors

90744eb

shrink allocation to decrease leak size

c3b7ebb

this should be reverted once a more sophisticated string mechanism in place until then this at least decreases the amount that is leaked

cleanup tests

b8fac4c

optimize String::from_bytes

7488494

We already call `StringBuilder::reserve_exact` so we already know the allocation cannot be shrinked further. This avoids an extra trip to the allocator in `String::from_bytes`.

add extra checks for debug mode

38ad425

add doc comment

3df32f5

remove redundant comment

2c507a6

airblast-dev marked this pull request as ready for review December 31, 2024 13:26

airblast-dev added 4 commits January 1, 2025 01:29

use malloc in String::with_capacity

ebc1b81

avoids extra computation and checks

add test for StringBuilder::with_capacity

7addd67

drop empty strings with reserved capacity

33a6b5a

fix off by one in alloc check

5d61731

no UB, but caused allocations when cap would fit in remaining

noib3 requested changes Jan 1, 2025

View reviewed changes

airblast-dev added 4 commits January 1, 2025 10:35

use with_capacity instead of reserve_exact

47b19a2

remove finish_unchecked

291934a

remove duplicate code in reserve*

4f84eac

reword remaining capacity docs

15a3b96

noib3 requested changes Jan 1, 2025

View reviewed changes

airblast-dev added 2 commits January 1, 2025 12:25

remove extra check

6f54bc5

fix typo

81d561c

airblast-dev and others added 9 commits January 2, 2025 11:57

remove redundant comment

2972fa7

remove capacity check

df371ab

fmt

6f6a6cd

add null check in finish

c816c7d

make finish public

2304160

fmt

8d464dc

not sure what changed

Format comments

a101287

Even though we have a `max_width` of 79, rustfmt doesn't enforce it in comments. I do.

Re-add commented out capacity check

f7b29d3

Rename reserve_raw to realloc

cb67a1c

noib3 merged commit f5f8fb5 into noib3:main Jan 2, 2025
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup `StringBuilder` methods #211

Cleanup `StringBuilder` methods #211

airblast-dev commented Dec 30, 2024

airblast-dev commented Dec 31, 2024

airblast-dev commented Dec 31, 2024 •

edited

Loading

noib3 left a comment

noib3 commented Jan 1, 2025

airblast-dev commented Jan 1, 2025

airblast-dev commented Jan 1, 2025 •

edited

Loading

noib3 commented Jan 1, 2025

airblast-dev commented Jan 1, 2025 •

edited

Loading

noib3 commented Jan 1, 2025

noib3 commented Jan 2, 2025

Cleanup StringBuilder methods #211

Cleanup StringBuilder methods #211

Conversation

airblast-dev commented Dec 30, 2024

airblast-dev commented Dec 31, 2024

airblast-dev commented Dec 31, 2024 • edited Loading

noib3 left a comment

Choose a reason for hiding this comment

noib3 commented Jan 1, 2025

airblast-dev commented Jan 1, 2025

airblast-dev commented Jan 1, 2025 • edited Loading

noib3 commented Jan 1, 2025

airblast-dev commented Jan 1, 2025 • edited Loading

noib3 commented Jan 1, 2025

noib3 commented Jan 2, 2025

Cleanup `StringBuilder` methods #211

Cleanup `StringBuilder` methods #211

airblast-dev commented Dec 31, 2024 •

edited

Loading

airblast-dev commented Jan 1, 2025 •

edited

Loading

airblast-dev commented Jan 1, 2025 •

edited

Loading