linux-kernel - Re: [PATCH] fixdep: handle short reads in read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87h6aqo8t2.fsf@gentoo.org>
Date: Sun, 08 Sep 2024 09:20:57 +0100
From: Sam James <sam@...too.org>
To: Masahiro Yamada <masahiroy@...nel.org>
Cc: Nathan Chancellor <nathan@...nel.org>,  Nicolas Schier
 <nicolas@...sle.eu>,  linux-kbuild@...r.kernel.org,
  linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fixdep: handle short reads in read_file

Masahiro Yamada <masahiroy@...nel.org> writes:

> On Sat, Sep 7, 2024 at 10:26 PM Sam James <sam@...too.org> wrote:
>>
>> Sam James <sam@...too.org> writes:
>>
>> > Masahiro Yamada <masahiroy@...nel.org> writes:
>> >
>> >> On Sat, Sep 7, 2024 at 12:29 AM Sam James <sam@...too.org> wrote:
>> >
>> > Hi Masahiro,
>> >
>> >>>
>> >>> 50% or so of kernel builds within our package manager fail for me with
>> >>> 'fixdep: read: success' because read(), for some reason - possibly ptrace,
>> >>> only read a short amount, not the full size.
>> >>>
>> >>> Unfortunately, this didn't trigger a -Wunused-result warning because
>> >>> we _are_ checking the return value, but with a bad comparison (it's completely
>> >>> fine for read() to not read the whole file in one gulp).
>> >>>
>> >>> Fixes: 01b5cbe7012fb1eeffc5c143865569835bcd405e
>> >>
>> >>
>> >> Fixes: 01b5cbe7012f ("fixdep: use malloc() and read() to load dep_file
>> >> to buffer")
>> >>
>> >
>> > Ah, thanks. I'll fix that and send v2 depending on how we decide to move
>> > forward wrt below.
>> >
>> >>
>> >> I guess, another approach would be to use fread() instead of read().
>> >>
>> >> Does the attached diff fix the issue too?
>> >>
>> >>
>> >
>> > Unfortunately no. It failed for me in the same way as before :(
>> >
>> > The man page mentions:
>> >> On  success, fread() and fwrite() return the number of items read or
>> >> written. This number equals the number of bytes transferred only when size is 1.
>> >
>> > so I guess it suffers from the same pitfall. I checked POSIX & ISO C as well
>> > which says:
>> >> If a partial element is read, its value is unspecified.
>> > and
>> >> The fread() function shall return the number of elements successfully
>> >> read, which shall be less than nitems only if an error or end-of-file
>> >> is encountered, or size is zero.
>> >
>> > The error reference is kind of mysterious there though.
>> >
>> > It kind of looks like fread *should* work. I'll send this mail and then
>> > think about it a bit later and ask around to see if I'm missing
>> > something obvious?
>>
>> OK, others disagree with my reading of fread and think it is ambiguous.
>>
>> With your patch, I was able to get failures albeit possibly less
>> frequently. I'm trying my patch again in a loop now.
>>
>> >
>> >> [...]
>> >
>> > thanks,
>> > sam
>>
>
>
>
>
>
>
> Your quotation of the POSIX fread() spec:
> (https://pubs.opengroup.org/onlinepubs/000095399/functions/fread.html)
>
>> If a partial element is read, its value is unspecified.
> and
>> The fread() function shall return the number of elements successfully
>> read, which shall be less than nitems only if an error or end-of-file
>> is encountered, or size is zero.
>
>
> I think this is clear enough.
>
>
> The end-of-file should not be encountered, as we check the file
> size in advance.

I believe that it's referring to the number of records, not if you read
*1* record of size N. I looked at the musl and glibc sources and neither
seem to retry partial reads in that case.

I don't see any error indicator set.