Fix segfault in FileChooserExtManual::add_choice() #1834
Merged
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.
Bug
The
FileChooserExtManual::add_choice
method includes some custom code, which is used to convert fromto the final two arguments of
gtk_file_chooser_add_choice
:This conversion is broken, and will segfault when passed anything other than an empty slice. See this repository for a minimal test program implemented in both C and Rust. The C version runs correctly, showing a dialog with both a boolean choice and a multiple value choice:
The Rust version segfaults in the
add_choice
call when trying to add the multiple value choice. Tested on Debian x86_64 withlibgtk-4-1
version4.14.4+ds-8
and Rust 1.80.History
This method was first implemented in PR #612, but that version had three bugs that I can see:
NULL, NULL
to get a boolean option).Vec
temporaries to be dropped, since only the return values ofVec::as_ptr
were kept in scope.Later, PR #1234 noted that this method was segfaulting, and fixed bug 1, the empty input case. It did not address the other two bugs. It also caused more of the required temporaries to be dropped, by placing them in an
else
block that ends before the C call.Fix
This PR fixes the method to work correctly for all cases, handling the empty and non-empty cases separately, keeping all required temporaries in scope for the non-empty case, and adding the necessary null terminations to the generated string arrays. Additionally, since everything except calling the C function is actually safe Rust, I have reduced the use of
unsafe
to the call sites only.The fix can be tested using the test case repository linked above.