zenity dialog: fix data race and reduce number of allocs

[marcin-serwin]

Description

dialog/unix: remove /usr/bin/env indirection

SDL process is using posix_spawnp which already does the path lookup so additional indirection via env is unnecessary

dialog/unix: reduce string allocations

Simplifies logic of spawning the zenity process by avoiding allocations of static strings

dialog/unix: stricter typing

Introduce const where appropriate

dialog/unix: deep copy args to avoid data race

SDL properties passed to the SDL_Zenity_ShowFileDialogWithProperties may be destroyed/modified immediately upon the function return. Therefore, we must copy all the strings and structures passed via pointers; otherwise, the SDL_ZenityFileDialog thread attempting to read them will cause data race.

Existing Issue(s)

N/A

[Semphriss]

Leaving a comment here as a reminder for myself to review this. I remember there were a few difficulties with that part of the code, and I'd like to make sure they weren't missed.

[marcin-serwin]

I noticed that the --attach argument is marked as deprecated in the man page of zenity and trying to pass it results in the following warning:

Warning: --attach is deprecated and will be removed in a future version of zenity. Ignoring.

Not sure why it was added recently. My zenity version is 4.0.3.

Side note: I think it would make sense to require some minimum version of zenity when checking for availability. This would allow us to remove the passing of ZENITY_* env variables and using SDL_CreateProcess instead of SDL_CreateProcessWithProperties which would further simplify the code. The version parsing is already done in src/video/wayland/SDL_waylandmessagebox.c so this code could be shared.

[slouken]

I noticed that the --attach argument is marked as deprecated in the man page of zenity and trying to pass it results in the following warning:

Warning: --attach is deprecated and will be removed in a future version of zenity. Ignoring.

Not sure why it was added recently. My zenity version is 4.0.3.

Side note: I think it would make sense to require some minimum version of zenity when checking for availability. This would allow us to remove the passing of ZENITY_* env variables and using SDL_CreateProcess instead of SDL_CreateProcessWithProperties which would further simplify the code. The version parsing is already done in src/video/wayland/SDL_waylandmessagebox.c so this code could be shared.

We don't know in advance what version of zenity will be available, and we potentially need to show an error dialog on an older system, so unfortunately we're stuck building a matrix of zenity versions and what command line arguments are valid. The fact that they deprecate and then remove parameters means that we're stuck in this situation.

[marcin-serwin]

We don't know in advance what version of zenity will be available, and we potentially need to show an error dialog on an older system

I think that error dialogs are covered by SDL_ShowSimpleMessageBox and we could write code that is more lenient when it comes to the zenity version there, while requiring modern zenity for file dialogs here.

[slouken]

Much better, thanks! I assume you've done testing to make sure the latest code covers whatever test case you had that discovered this issue?

[marcin-serwin]

I assume you've done testing to make sure the latest code covers whatever test case you had that discovered this issue?

I've discovered it by reading the code but I checked it against a minimal program. Depending on the planet alignment it either works correctly, shows garbled text, fails to open zenity due to it parsing garbled text, or segfaults on main and 3.1.6 but runs reliably on this branch.

Example code

#define SDL_MAIN_USE_CALLBACKS
#include <SDL3/SDL.h>
#include <SDL3/SDL_main.h>

static SDL_Window *window;
static SDL_Renderer *renderer;

SDL_AppResult SDL_AppInit(void **data, int argc, char **argv) {
  SDL_InitSubSystem(SDL_INIT_VIDEO);
  SDL_CreateWindowAndRenderer("foo", 800, 600, 0, &window, &renderer);
  return SDL_APP_CONTINUE;
}

SDL_AppResult SDL_AppIterate(void *data) {
  SDL_RenderClear(renderer);
  SDL_Delay(100);
  SDL_RenderPresent(renderer);
  return SDL_APP_CONTINUE;
}

void callback(void *data, const char *const *file, int foo) {
  if (file) {
    SDL_Log("ok: %p %s %d", data, *file, foo);
  } else {
    SDL_Log("err: %s", SDL_GetError());
  }
}

SDL_AppResult SDL_AppEvent(void *data, SDL_Event *event) {
  switch (event->type) {
  case SDL_EVENT_QUIT:
    return SDL_APP_SUCCESS;
  case SDL_EVENT_KEY_DOWN:
    switch (event->key.scancode) {
    case SDL_SCANCODE_F: {
      SDL_DialogFileFilter filters[] = {
          {.name = "PNG", .pattern = "png"},
      };
      SDL_ShowOpenFileDialog(&callback, NULL, window, filters, 1, NULL, false);
    }
    }
  }
  return SDL_APP_CONTINUE;
}

void SDL_AppQuit(void *data, SDL_AppResult result) {}

[Semphriss]

For information, in #11170 I added some documentation that states that filters must remain valid at least until the callback is invoked.

[marcin-serwin]

For information, in #11170 I added some documentation that states that filters must remain valid at least until the callback is invoked.

I didn't notice that; I guess that we can avoid deep-copying filters then. The other strings still need to be copied though since they are freed when the wrapper functions destroy the properties.

[Semphriss]

I don't mind changing the behavior to remove that requirement if it can help avoid some mistakes. I expect people not to notice that the filters must remain valid and inadvertently write buggy code as you showed, so less restrictions would be better, as long as the extra complexity is worth it.

I haven't yet had an opportunity to review the PR in-depth, but I'll try to clear myself some time in a few hours.

[marcin-serwin]

Now that I think about it: Is there any reason why the argv creation must be done on the spawned thread? We could both avoid deep-copying filters and requiring them to be held until callback by simply creating the --file-filter=... strings on the main thread and then passing the Args in the zenityArgs.

[marcin-serwin]

I don't mind changing the behavior to remove that requirement if it can help avoid some mistakes.

Since you worked on the other dialogs: Is zenity the only one that required that or do the other also need adjustments?

[slouken]

I don't mind changing the behavior to remove that requirement if it can help avoid some mistakes. I expect people not to notice that the filters must remain valid and inadvertently write buggy code as you showed, so less restrictions would be better, as long as the extra complexity is worth it.

Yes please, let's remove that requirement as it'll trip a bunch of people up, and allocation in this code is fine. (but can be done in a separate PR)

[slouken]

Now that I think about it: Is there any reason why the argv creation must be done on the spawned thread? We could both avoid deep-copying filters and requiring them to be held until callback by simply creating the --file-filter=... strings on the main thread and then passing the Args in the zenityArgs.

Yes, this seems like the right thing to do.

[Semphriss]

I don't mind changing the behavior to remove that requirement if it can help avoid some mistakes.

Since you worked on the other dialogs: Is zenity the only one that required that or do the other also need adjustments?

I believe all of those that use threads in some capacity have the same requirement, but the filters can be cloned (or possibly convert_filters()'ed) early the same way. I'll give it a quick look in a moment.

[slouken]

@Semphriss, let's merge this as-is, and come back to the filters as a separate commit?

[Semphriss]

I'll give it one last quick look before merging, but I'm fine with making a separate PR for filter duplication.

[slouken]

Merged, thanks!