BestImageViewer / geeqie

claiming to be the best image viewer / photo collection browser
http://www.geeqie.org/
GNU General Public License v2.0
488 stars 79 forks source link

[Patch attached] Geeqie fails to copy files with file name length close to linux maximum #1544

Open installgentoo opened 3 days ago

installgentoo commented 3 days ago

Setup (please complete the following information):

Describe the bug Ditto.

To reproduce Name a file with 255 characters, try to copy it. Get a nondescript error.

Additional context This happens because of

    std::unique_ptr<gchar, decltype(&g_free)> randname{g_strconcat(tl.get(), ".tmp_XXXXXX", NULL), g_free};

at src/ui-fileops.cc:648

I assume this is meant to safeguard against partial operations that get interrupted, ironic that the safety measure itself caused a bug. See attached patch with a fix, refactor it into geeqie code standard and merge in. filenamelen.patch

qarkai commented 2 days ago

Could you create a PR for this patch?

qarkai commented 2 days ago

ironic that the safety measure itself caused a bug

Could you give more details? I don't see how unique_ptr wrapper gives nondescript error.

installgentoo commented 2 days ago

@qarkai

appending ".tmp_XXXXXX" to filename extends the filename length past 255 characters.

qarkai commented 2 days ago

@qarkai

appending ".tmp_XXXXXX" to filename extends the filename length past 255 characters.

So IIUC any g_strconcat and g_build_filename is dangerous. Also it has nothing to do with memory safety.

qarkai commented 2 days ago

@caclark should we apply wrapper from patch to all relevant g_strconcat and g_build_filename code?

installgentoo commented 2 days ago

@qarkai is there any other code that appends arbitrary strings to filenames?

If so i would think my check+truncate approach should be a function and used everywhere.

qarkai commented 2 days ago

@qarkai is there any other code that appends arbitrary strings to filenames?

If so i would think my check+truncate approach should be a function and used everywhere.

Proposed patch looks like overengineering. Should be enough randname = g_build_filename(tl_dirname, ".tmp_XXXXXX", NULL);. Again, could you create PR? Note that unique_ptr was replaced with g_auto* macros.

qarkai commented 2 days ago

@installgentoo Is it related to #944?

installgentoo commented 2 days ago

@qarkai must be. Please write PR or whatever and close that other bug.