Re: [glibc PATCH] fcntl: put F_OFD_* constants under #ifdef __USE_FILE_OFFSET64

[Florian Weimer <fweimer@redhat.com>]

On 08/17/2016 08:21 PM, Jeff Layton wrote:
> On Wed, 2016-08-17 at 20:02 +0200, Florian Weimer wrote:
>> On 08/17/2016 07:39 PM, Jeff Layton wrote:
>>>
>>> On Wed, 2016-08-17 at 19:34 +0200, Florian Weimer wrote:
>>>>
>>>> On 08/17/2016 04:47 PM, Jeff Layton wrote:
>>>>>
>>>>>
>>>>> The Linux kernel expects a flock64 structure whenever you use
>>>>> OFD locks
>>>>> with fcntl64. Unfortunately, you can currently build a 32-bit
>>>>> program
>>>>> that passes in a struct flock when it calls fcntl64.
>>>>>
>>>>> Only define the F_OFD_* constants when __USE_FILE_OFFSET64 is
>>>>> also
>>>>> defined, so that the build fails in this situation rather than
>>>>> producing a broken binary.
>>>>
>>>> Doesn't this affect legacy POSIX-style locks as well, under very
>>>> similar
>>>> circumstances?
>>>>
>>>>
>>>
>>> No. The kernel will decide which type of struct it is based on
>>> whether
>>> userland passes in F_SETLK or F_SETLK64.
>>
>> Let me see if I can sort this out.  Is the situation like this?
>>
>>          _FILE_OFFSET_…    …BITS == 32          …BITS == 64
>>          struct …       flock   flock64    flock   flock64
>> fcntl (F_SETLK)        ok      BAD        ok      BAD
>> fcntl (F_SETLK64)      BAD     ok         ok      ok
>> fcntl (F_OFD_SETLK)    BAD     ok¹        ok      ok
>>
>> ¹ is broken by your patch, right?
>
> Not sure I 100% understand your chart, but if I do then I think it's
> more like:
>
>          _FILE_OFFSET_…    …BITS == 32          …BITS == 64
>          struct …       flock   flock64    flock   flock64
> fcntl (F_SETLK)        ok      BAD        ok      ok
> fcntl (F_SETLK64)      BAD     ok         ok      ok
> fcntl (F_OFD_SETLK)    BAD     ok¹        ok      ok
>
> struct flock and struct flock64 are generally equivalent when
> _FILE_OFFSET_BITS==64.

Why would the F_SETLK operation work with a struct flock64 in 
_FILE_OFFSET_BITS == 64 mode?  I think the kernel still expects a 32-bit 
struct.

glibc does not look at O_LARGEFILE and alters size expectations. 
Neither does the kernel.

> I don't quite understand how ¹ would be broken by this patch. The idea
> with the patch is to ensure that if you haven't defined
> _FILE_OFFSET_BITS=64 on a 32 bit arch, that it's broken at compile time
> instead of at runtime.

Compile time breakage is still breakage.  I want to avoid another 
strerror_r situation where it's very hard to get the job done due to the 
way the preprocessor conditionals work out.

>> Looking at the definition of struct flock and struct flock64, the
>> risk
>> is that application silently succeed in locking the wrong thing when
>> using struct flock64 with a 32-it interface.
>>
>
> Yes. The basic problem is that the kernel will expect a struct flock64,
> but if you don't set _FILE_OFFSET_BITS=64 glibc will pass in a legacy
> struct flock instead. The kernel can then read beyond the end of the
> struct.
>
> The bytes in l_start and l_len will be slurped into the kernel's
> l_start field. The pid and whatever junk is beyond the struct will be
> in the l_len and pid fields.
>
> It's also possible the program will get back EFAULT as well if
> copy_from_user fails.

I was mainly worried about the reverse case (calling 32-bit fcntl with 
struct flock64).  But this cannot happen because glibc always calls 
fcntl64 on 32-bit architectures.

Florian