On 19/04/23 16:40, Sergey Bugaev wrote:
> On Wed, Apr 19, 2023, 22:13 Cristian Rodríguez <crrodrig...@opensuse.org>
> wrote:
>> Im a little bit lost on what it was supposed to do in this old form.. as
>> the open flags are all wrong..
>>
>> Changelog says:
>>
>> (__libc_check_standard_fds): Reverse modes so that common operations on
>> the descriptors fail.
>>
>> So this was intended at some point in the past decades to make it fail..
>
> Ah, I see, so I just failed to trace it through git blame / git log,
> because the commit renaming this file sysdeps/generic/check_fds.c ->
> csu/check_fds.c has been done wrong (or maybe this is an artefact of
> the migration to Git).
>
> So which way would you prefer it, left as-is (i.e. without this patch)
> or switched back? If we leave it as is, we should at least add a
> comment explaining what's going on, for the next person who stumbles
> into this and also fails to trace it through git blame.
>
>> I see it is used only for SUID statically linked binaries.
>
> I might be missing something, but why statically linked only? I don't
> see anything like that in elf/Makefile (but maybe I don't know where
> to look, please tell me!), and also the same behavior is certainly
> exhibited by dynamically linked executables too. That ls -l I posted
> above is from a dynamic executable.
At least on Hurd, __libc_check_standard_fds is only built for !SHARED.
>
>> is this really needed now? playing silly games with this fds will always
>> result in silly prices.
My understanding of this code is to enforce that on setuid program with
stdin/stdout/stderr closed any operation fail. The original commit
db33f7d4aef7 that added these had this specific comment:
/* Protec SUID program against misuse of file descriptors. */
extern void __libc_check_standard_fds (void);
Maybe we can add extend the comment on this file to add the intention
of this code.
