Re: [PATCH] Fix #27777 - now use a doubly-linked list for _IO_list_all

[Florian Weimer <fweimer@redhat.com>]

* alexandre ferrieux:

> On 26/04/2024 20:50, H.J. Lu wrote:
>>
>> On Fri, Apr 26, 2024 at 10:51 AM Florian Weimer <fweimer@redhat.com> wrote:
>>>
>>> * alexandre ferrieux:
>>>
>>> > diff --git a/libio/bits/types/struct_FILE.h b/libio/bits/types/struct_FILE.h
>>> > index 7cdaae86f8..daebd6ce0a 100644
>>> > --- a/libio/bits/types/struct_FILE.h
>>> > +++ b/libio/bits/types/struct_FILE.h
>>> > @@ -67,7 +67,7 @@ struct _IO_FILE
>>> >
>>> >    struct _IO_marker *_markers;
>>> >
>>> > -  struct _IO_FILE *_chain;
>>> > +  struct _IO_FILE *_chain,**_prevchain;
>>> >
>>> >    int _fileno;
>>> >    int _flags2;
>>>
>>> Unfortunately, this struct is part of the ABI for several reasons.  For
>>> example, _IO_2_1_stdin_ is a global variable exported by libc.so.6, and
>>> it has a struct _IO_FILE member.  It cannot change size due to copy
>>> relocations, even if nothing accesses members after _chain.
>>>
>>> I disagree with H.J.'s suggestion that the answer is symbol
>>> versions. 8-)
>> libio/bits/types/struct_FILE.h has
>>    __off64_t _offset;
>>    /* Wide character stream stuff.  */
>>    struct _IO_codecvt *_codecvt;
>>    struct _IO_wide_data *_wide_data;
>>    struct _IO_FILE *_freeres_list;
>>    void *_freeres_buf;
>>    size_t __pad5;
>>    int _mode;
>>    /* Make sure we don't get into trouble again.  */
>>    char _unused2[15 * sizeof (int) - 4 * sizeof (void *) - sizeof (size_t)];
>> };
>> We can add a pointer without changing FILE size.
>
> Ha ! That's a clever trick. Presumably someone was already bitten once ;)
> (Sorry I didn't notice it was present in your corrected patch).
>
> Now, I still have worries about the symbol versioning approach: why
> restrict the version bump to _IO_un_link and _IO_link_in ? Indeed, any
> program calling fclose()  ends up calling _IO_un_link(), so the
> version check should apply too, right ? So shouldn't we tag all
> functions using (FILE *) in their arguments or return value ?

No new symbol versions should be needed.  You just need to be careful
not to access that part of the struct when it does not exist.

I think the way to check for old-style streams is via _IO_vtable_offset,
as can be seen at the start of the __fgetln function in an (unmerged)
patch I submitted a while back:

  [PATCH] stdio-common: Add the fgetln function
  <https://inbox.sourceware.org/libc-alpha/871qxbxe2i.fsf@oldenburg.str.redhat.com/>

In your case, you would have to do the full traversal once you encounter
a missing backlink.  But that would only happen if old-style file
handles are actually used.  Maybe this is even easier to implement than
the hash table or index approach.

Thanks,
Florian