mbox series

[v4,0/6] fcntl fortification

Message ID 20230730192605.2423480-1-bugaevc@gmail.com
Headers show
Series fcntl fortification | expand

Message

Sergey Bugaev July 30, 2023, 7:25 p.m. UTC 
Hello,

this is the v4 of the fcntl fortification work. v1 was at [0], v2 at
[1], v3 at [2].

[0]: https://sourceware.org/pipermail/libc-alpha/2023-May/148332.html
[1]: https://sourceware.org/pipermail/libc-alpha/2023-May/148569.html
[2]: https://sourceware.org/pipermail/libc-alpha/2023-June/149096.html

Changes since v3:

- Rebased onto the latest master.
- Fixed the bug found by Linaro CI (thanks!): F_DUPFD_CLOEXEC is not
  always defined, so it has to be surrounded by ifdef checks too.
- This patchset is now compatible with Frédéric Bérat's work on
  fortifying glibc itself / --enable-fortify-source!
- Fixed a bug: F_GETLK etc may have the same values as F_GETLK64 etc, so
  the previous version of this patchset would complain about F_GETLK64
  usage with struct flock64, since the check for F_GETLK happened first.
  This is now fixed by accepting both struct flock and struct flock64 if
  F_GETLK has the same value as F_GETLK64.
- Found and fixed a few cases of what seems to be actual commmand / type
  confusion in the tests! Specifically, it was calling
  fcntl64 (fd, F_SETLK, &flock64)
  in a few places, which is incorrect according to my understanding and
  my tests of Linux behavior. Please see the first patch for some more
  details, and please correct me if I'm wrong!

I've checked that this builds and passes tests (there are a few test
failures, but they all seem unrelated) for x86_64-linux-gnu and
i686-linux-gnu with and without --enable-fortify-source. I've also
checked that it builds for x86_64-gnu with and without
--enable-fortify-source, but I haven't run the tests.

Sergey

Comments

Zack Weinberg July 31, 2023, 2:40 p.m. UTC | #1 
On Sun, Jul 30, 2023, at 3:25 PM, Sergey Bugaev via Libc-alpha wrote:
> this is the v4 of the fcntl fortification work.

I apologize if this has already been discussed, but I can't find any mention of it. What does this patch do with code that supplies an *unnecessary* third argument to fcntl and/or open? (For example, `open(fname, O_RDONLY, 0)`.

I have seen this fairly often and it's harmless, so I think it should probably continue to be allowed. I can see an argument for warning about this, but I think that belongs in the compiler, with a dedicated -W option to squelch it.

zw
Adhemerval Zanella Netto July 31, 2023, 5:25 p.m. UTC | #2 
On 31/07/23 11:40, Zack Weinberg via Libc-alpha wrote:
> On Sun, Jul 30, 2023, at 3:25 PM, Sergey Bugaev via Libc-alpha wrote:
>> this is the v4 of the fcntl fortification work.
> 
> I apologize if this has already been discussed, but I can't find any mention of it. What does this patch do with code that supplies an *unnecessary* third argument to fcntl and/or open? (For example, `open(fname, O_RDONLY, 0)`.
> 
> I have seen this fairly often and it's harmless, so I think it should probably continue to be allowed. I can see an argument for warning about this, but I think that belongs in the compiler, with a dedicated -W option to squelch it.

My understanding reviewing the previous revision is the other way around:
like __open_2 the idea is warn if the fcntl requires a third argument
based on second argument value.  Like fortified open, adding an extra
argument where it is not required should not trigger a fortify issue.