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 segfault in FileChooserExtManual::add_choice() #1834

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

martinling
Copy link
Contributor

Bug

The FileChooserExtManual::add_choice method includes some custom code, which is used to convert from

options: &[(&str, &str)]

to the final two arguments of gtk_file_chooser_add_choice:

const char **options, const char **option_labels

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:

image

The Rust version segfaults in the add_choice call when trying to add the multiple value choice. Tested on Debian x86_64 with libgtk-4-1 version 4.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:

  1. It did not handle the empty slice case (which must be passed as NULL, NULL to get a boolean option).
  2. It was missing the required null terminations for each array in the non-empty slice case.
  3. It allowed its final pair of Vec temporaries to be dropped, since only the return values of Vec::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.

@sdroege sdroege merged commit aeaa22c into gtk-rs:master Sep 9, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants