mpartel / bindfs

Mount a directory elsewhere with changed permissions.
https://bindfs.org/
GNU General Public License v2.0
436 stars 64 forks source link

[1.17.5] Commit 38cd6e7e13ce966dc8e196eb3037935682e613a6 introduced undefined behavior? #143

Closed hartwork closed 10 months ago

hartwork commented 10 months ago

Hi @mpartel,

when bumping bindfs to 1.17.5 in Gentoo I noticed that the code from commit 38cd6e7e13ce966dc8e196eb3037935682e613a6 apparently introduced undefined behavior through signed integer overflow here:

https://github.com/mpartel/bindfs/blob/3f57fa69448ad68f31cf62cf75b5a9b2e5096c05/src/bindfs.c#L639-L640

To avoid undefined behavior, the check for overflow needs to happen prior to that addition, not after. The same applies to the group ID code a few lines down and also to the unapply editions, so four occurrences in total.

It is worth to note that I'm assuming uid_t and gid_t to be signed integers here in line with this comment: https://github.com/mpartel/bindfs/blob/3f57fa69448ad68f31cf62cf75b5a9b2e5096c05/src/bindfs.c#L118-L119

Should I make a pull request to address the issue?

Best, Sebastian

mpartel commented 10 months ago

Argh, should never C while sleepy :man_facepalming: I even briefly thought about this, but made the mistake of assuming unsignedness anyway. Thank you for the keen eye!

uid_t and gid_t do not appear to be signed on x86_64 Linux, but I'll fix this anyway.

mpartel commented 10 months ago

Fixed with 1.17.6

hartwork commented 10 months ago

@mpartel thanks for the quick fix, bumped to 1.17.6 in Gentoo also now.