Re: 5.8: LTO exposes some new issues

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

Roman Perepelitsa wrote on Tue, 28 Jul 2020 12:52 +0200:
> On Tue, Jul 28, 2020 at 10:26 AM Peter Stephenson
> <p.w.stephenson@xxxxxxxxxxxx> wrote:
> >  
> > > On 28 July 2020 at 08:53 Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > It's clearly correct, but as written, the patch loses the distinction
> > > that these members are private to hashtable.c and should not be accessed
> > > by other parts of the code.  Could you address that, please?  If
> > > there's an easy way to have the compiler enforce this restriction,
> > > great; else, we can at least add a comment.  
> >
> > One way is to have a "struct { ... } private" substructure,
> > which it makes it clear what's going on within the code (though comments
> > are obviously useful, too).  
> 
> How about this? The diff is a bit larger but the code is fairly
> straightforward. Only hashtable.c has access to internal fields, just
> like before the patch.
> 
> In a nutshell, struct hashtable has only public data members. Within
> hashtable.c there is struct hashtableimpl, which has struct hashtable
> as the first data member. C allows casting a pointer to a struct to a
> pointer to its first data member and back without violating aliasing
> rules. Thus hashtable.c can cast struct hashtable* to struct
> hashtableimpl* in order to get access to internal fields.

Thanks, that addresses the previous point, but unfortunately it creates
another problem: people who read the .h file are liable to declare
local variables of type 'struct hashtable', or memcpy() them around,
and in either case, once such a variable gets to hashtable.c and the
private members are accessed, we'll get out-of-bounds reads.

So we need either a comment at the definition of the struct type that
says nobody should allocate/duplicate/assign such structs directly, but
call newhashtable() instead, or a solution that doesn't involve casts,
such as Peter's proposal.

Cheers,

Daniel