No read permissions on thumbnails [patch included]

[installgentoo]

Setup (please complete the following information):

• Distribution: gentoo
• Distribution release: master
• Geeqie version [geeqie --version]: 2.4

Describe the bug In 2.4 geeqie creates thumbnails with permission 000 for some reason. As a result, nothing, not even geeqie can read them. Otherwise thumbnails work the same as previously.

To reproduce Steps to reproduce the behavior: Build without exif Set thumbs to ~/.cache/thumbnails/

[installgentoo]

What the actual fuck are these names?

static void vf_thumb_common_cb(ThumbLoader *tl, gpointer data)
{
    auto vf = static_cast<ViewFile *>(data);

    if (vf->thumbs_filedata && vf->thumbs_loader == tl)
        {
        vf_thumb_do(vf, vf->thumbs_filedata);
        }

    while (vf_thumb_next(vf));
}

Is your effort to refactor(codegolf?) geeqie aimed at making code as impenetrable as possible? There are also callbacks and builders everywhere, just to obfuscate things a little bit further.

Old thumbs were created via "thumb_loader_save_thumbnail". Observe the difference in naming.

Please watch this: https://www.youtube.com/watch?v=8ncQrGuunHY the man neatly summarises how2code

[installgentoo]

The offender is geeqie-2.4/src/thumb-standard.cc:372

chmod(pathl, (tl->cache_local) ? tl->source_mode : S_IRUSR & S_IWUSR);

Specifically i = S_IRUSR & S_IWUSR results in i = 0 for me. Probably some includes are missing or something. This is a horrible anti-pattern and just don't use macros, okay? Set a local const.

        if (success)
            {
            const auto default_permission =0755;
            chmod(pathl, (tl->cache_local) ? tl->source_mode : default_permission);

No one outside this bit of code needs to know about those defines, especially seeing as their names are gibberish.

[installgentoo]

patch

--- a/src/thumb-standard.cc 2022-04-12 03:36:50.000000000 +1200
+++ b/src/thumb-standard.cc 2022-05-07 02:24:19.136982399 +1200
@@ -369,7 +369,8 @@
                      NULL);
        if (success)
            {
-           chmod(pathl, (tl->cache_local) ? tl->source_mode : S_IRUSR & S_IWUSR);
+           const auto default_permission = 0600;
+           chmod(pathl, (tl->cache_local) ? tl->source_mode : default_permission);
            success = rename_file(tmp_path, tl->thumb_path);
            }

[xsdg]

Hi, thanks for reporting and then identifying this bug. The actual issue is that S_IRUSR & S_IWUSR should be S_IRUSR | S_IWUSR. Because S_IRUSR and S_IWUSR represent distinct bits in the permissions bitfield, ANDing them will always return 0. I'll file a pull request with that change.

To respond to some of the other comments, those constants (not macros) are defined here: \ https://pubs.opengroup.org/onlinepubs/7908799/xsh/sysstat.h.html \ Their names are a little hard to follow at first, but that's just because unix permissions can be a little hard to follow. How I understand these names is S_IRUSR -> "[Stat]_[Interface][Read][User]". So the bit set in that constant represents the user-read permission within the stat interface. Likewise, S_IWUSR is the [W]rite permission.

And vf_thumb_common_cb is a common naming convention in geeqie, which comes from it starting off life as a C (and not C++) project around 20 years ago. How to understand that name is "[ViewFile]_[thumbnail]_[common]_[callback]". So it's a common callback that's related to ViewFile thumbnails.

[installgentoo]

Geeqie had bugs on every single release thus far. I think it's about time to move past C heritage. From what i saw for the last 10 years, move to cpp have only served to complicate things further, not to fix any of the issues(still lagging, still using god-awful gtk "async"). I mean you guys don't need employment security in this repo, why do you insist on using the worst practices imaginable?

[caclark]

The project did not have a Code of Conduct, so I have included a very basic one, mostly copied from another project. It is on the main project page. Comments are welcome, of course.

[mowgli]

Hi Colin,

Am Mi den 26. Jun 2024 um 19:19 schrieb Colin Clark:

The project did not have a Code of Conduct, so I have included a very basic one, mostly copied from another project. It is on the main project page. Comments are welcome, of course.

Personal I do not like the idea to add that type of text to all projects. In many cases it is used to kick out unpopular people instead of including people. More over, projects adding it, often suffer from people not telling their opinion anymore as they fear that it will be against the coc.

That is the reason I always resisted to add such, I would tell it, crap.

The better in my opinion is to speak with each other and even allow swearing if it gets the heat cooled. :-)

However, that is my personal opinion, I am not the main contributor anymore. So feel free.

Regards Klaus -- Klaus Ethgen http://www.ethgen.ch/ pub 4096R/4E20AF1C 2011-05-16 Klaus Ethgen @.***> Fingerprint: 85D4 CA42 952C 949B 1753 62B3 79D0 B06F 4E20 AF1C

[installgentoo]

You guys, i did not mean to start a flame war between core contributors.

Colin's chastisement of me is noted, but i hope he also hears what I'm saying.

[qarkai]

How I understand these names is S_IRUSR -> "[Stat]_[Interface][Read][User]".

A small clarification, I stands for Inode https://stackoverflow.com/questions/14055755/whats-the-meaning-of-i-in-s-irusr

[installgentoo]

How I understand these names is S_IRUSR -> "[Stat]_[Interface][Read][User]".

A small clarification, I stands for Inode https://stackoverflow.com/questions/14055755/whats-the-meaning-of-i-in-s-irusr

yes, i too can kinda understand the names, obviously i'm exaggerating.

My point is thus: Enterprise software is written to fit an arbitrary spec largely because management wants to appear useful. "We designed the spec and software fits it". Thus best practices, clean code, oop, etc.

Good software should just aim to keep all relevant functionality as simple as possible while being as fast as needed. Simplicity also means that you want to make the code as self-explanatory as possible and as localised as possible. If i have to jump through 10 different places in a 4000 line file to understand how something actually works(and i literally have to debug geeqie every time, because it's impossible to keep track of all the callbacks going in and out of each other), i'm not gonna contribute, and people that contribute will never be able to get software working well, which is evidenced by every release having bugs that break something. You can see me constantly kvetch about new releases on this issue tracker.

React, which i hope everyone here agrees is a good framework, isn't good because it's super complex wizardry built on dom, and it's not good because it adheres to some sort of spec or best practices. It's good because it allows programmers to write self-contained functions that are easy to reason about, you can reason about them as simplistic single-threaded functional code. Wherein GTK's concurrency is awful 'cause you have to manually schedule stuff and make sure your functions don't take too long and eyeball your concurrency's correctness. And you have to keep track of it in every simplest function.

So, in this specific case, it is better to literally just have a constant that says "default_permission = 0600", locally where permission is used. If some other part of the code sets a different permission - that doesn't matter, you guys don't need to adhere to strict spec of what permissions on thumbnails or whatever else you set, and if it becomes a problem, it will be easy to find the relevant piece of code and change the permission.

[qarkai]

So, in this specific case, it is better to literally just have a constant that says "default_permission = 0600", locally where permission is used.

What is default_permission = 0600? I don't understand it. Does it mean that file could be opened at 6:00 AM exactly?

[installgentoo]

So, in this specific case, it is better to literally just have a constant that says "default_permission = 0600", locally where permission is used.

What is default_permission = 0600? I don't understand it. Does it mean that file could be opened at 6:00 AM exactly?

Fair point, but in this case it will be comprehensible from context.

chmod(pathl, (tl->cache_local) ? tl->source_mode : default_permission);

You can also say "but how will the contributors know when the context is appropriate"? And it is a good point, with junior devs. What i've done professionally is pass junior's work through our seniors that can understand the context. You could respond to this in that no one on this repo wants to be a code reviewer for people that may or may not commit, and it's much easier to have code policy, but that's sort of a cope-out. Engineering is reliant on context, since we're building complex systems. If you create a policy for some part of your engineering to offload decision-making, you'll inevitably end up with bad systems, if your problem isn't 100% the same as previous problem that the policy was based on. All the malpractices of enterprise show this in code bloat, boilerplate, slow software, having to constantly do bugfixing in the new release 9.10.177rc1

[mowgli]

Hi Arkadiy,

Am Do den 27. Jun 2024 um 12:21 schrieb Arkadiy Illarionov:

So, in this specific case, it is better to literally just have a constant that says "default_permission = 0600", locally where permission is used.

What is default_permission = 0600? I don't understand it. Does it mean that file could be opened at 6:00 AM exactly?

That depends.

First of all, that number is an octal number, the leading "0" does indicate that.

With that in mind, there are 3 (relevant) numbers. The first (6) is for the user to have read and write permissions (each one bit, there is another that is not set, meaning execution). The second is for the user group, the user is in (0 = no rights) and the last is for anybody else.

But there is even more. I have to have a look how it is implemented. Usually it also take the umask setting into the consideration. umask is inverted and give the bits that are alowed. Mine is 0027, which mean for user (0) I have no limitation, for group (2) I allow the bits for read and execution but not for write and for others (7) I don't allow any bit to be set. So in case of 0600 + 0027, the resulting permissions would be

600. But when set to 0666 + 0027, the resulting permissions would be

Regards Klaus -- Klaus Ethgen http://www.ethgen.ch/ pub 4096R/4E20AF1C 2011-05-16 Klaus Ethgen @.***> Fingerprint: 85D4 CA42 952C 949B 1753 62B3 79D0 B06F 4E20 AF1C

[xsdg]

@installgentoo For better or worse, Geeqie is a 130,000-line codebase. For better or worse, it was originally written in C, 20+ years ago. I ported it to C++. We are not going to rewrite it from scratch, so the only other option is incremental improvement, which is the path that every maintainer and contributor has taken in those 20 years.

It's clear that you're not a fan of gtk and glib. That doesn't matter, because Geeqie is sticking with gtk — switching UI toolkits would require a pretty significant rewrite, and again, we're not going to rewrite it. It's clear that you're not a fan of the coding style. That makes sense, since the original style is 20+ years old. The best approach is to maintain consistency and change things incrementally, which both @qarkai and @caclark have been doing an amazing job of.

Does Geeqie have bugs? Absolutely. Every project of this size has bugs. And the more things change, the more bugs are written. That's just how it goes.

At the same time, we are also working on adding validation that hasn't previously existed in the history of the project. @caclark set up the GitHub automatic compilation and static analysis checks, which have been an extraordinary asset for detecting issues and maintaining a code quality standard before commits are merged. I built on that by creating a (still fledgling) unit test suite. @qarkai has been a wizard with using clang-format to make incremental changes across the entire codebase. There's obviously a lot more to be done here, and we're making progress.

Lastly, the recommendation to use const auto default_permission = 0600; is just not a good recommendation. The chmod function is part of sys/stat.h, and the constants that we're using (also defined in sys/stat.h) are specified to be used with that function. See: \ https://man7.org/linux/man-pages/man2/chmod.2.html

Using bitwise operations with those constants is the intended and documented way to use the API:

The new file mode is specified in mode, which is a bit mask created by ORing together zero or more of the following…

The number 0600 is not inherently meaningful, except as being a bitwise OR of 0200 and 0400, with meanings that are already conveyed by the constants we've already been using. So avoiding the use of those documented constants by using a locally-defined magic constant just obscures what's actually going on, and also goes against the documented way to use the API.

[xsdg]

Usually it also take the umask setting into the consideration. umask is inverted and give the bits that are alowed. Mine is 0027, which mean for user (0) I have no limitation, for group (2) I allow the bits for read and execution but not for write and for others (7) I don't allow any bit to be set. So in case of 0600 + 0027, the resulting permissions would be 0600. But when set to 0666 + 0027, the resulting permissions would be 0640.

This is implemented by bitwise AND with the inverse of the umask. So:

$ruby -e 'perm = 0600; umask = 0027; puts (perm & ~umask).to_s(8)'
600

$ruby -e 'perm = 0666; umask = 0027; puts (perm & ~umask).to_s(8)'
640

[xsdg]

1412 was merged.