Re: CVS commit: src/sys/kern

[Valery Ushakov <uwe%stderr.spb.ru@localhost>]

On Thu, Nov 07, 2019 at 09:58:06 -0600, David Young wrote:

> On Thu, Nov 07, 2019 at 04:26:51PM +0100, Martin Husemann wrote:
> > On Thu, Nov 07, 2019 at 02:53:08PM +0100, Kamil Rytarowski wrote:
> > > On 07.11.2019 14:25, Valery Ushakov wrote:
> > > > If the sanitizer does complain about other uses, there is little point
> > > > in fixing one instance and not the others.
> > > 
> > > We already agreed with Christos that this is appeasing of GCC. If you
> > > want to scan the whole kernel (or whole C) file for more occurrences of
> > > violations - please go for it.
> > 
> > No. The commit needs to be reverted, and then
> > 
> >  a) either the root cause for the unaligned address be fixed or
> >  b) some other means be found to make the sanitizer shut up
> > 
> > As uwe said: papering over a tiny detail that *never* hits in the real
> > world but potentialy hiding a real issue is not the way to go.
> > 
> > Martin
> > 
> > P.S.: Independend of this I would still like an official C standard
> > clarification; in my reading a simple address calculation is not
> > accessing an object through a pointer (which would be the undefined
> > behaviour). If the C standard is not clear on this, it needs to be
> > improved.
> 
> I think the problem is that if you have the series of statements,
> 
>         element_t *e = &s->element;
> 
>         if (s == NULL)
>                 return;
> 
> then the comparison with NULL implies that in this scope, s could
> be NULL.  NULL does not necessarily have any "arithmetic" relationship
> to any other pointer---by that rationale, you probably cannot assign
> any alignment to it---so there is no sensible value that you can
> give to e.

This is not what the changed code does.  The code in question has

  struct disklabel *dlp = ...;

apparently gcc complains about

  memcmp(&dlp->d_magic, ...)

but later the code uses e.g. dlp->d_partitions (right after the
check_label_magic call) and other memebers.  So it's very suspicious
that one usage is flagged and others are not.

Until very recently the magic check was also explicitly comparing
dlp->d_magic != DISKMAGIC, etc.  So may be we should stop pretending
and rewrite check_label_magic() to use that instead of memcmp.  (And
then fix all dlp->foo in one swoop).

If my I interpretation is wrong, I would be glad to be corrected.


> There is probably an argument to be made that in a
> segmented/tagged/capability architecture that has run C programs
> (8086; Burroughs Large Systems) or may run them in the future (CHERI),
> &(NULL)->element cannot sensibly be computed.

Amen :).  I actually did encounter problems like that when compiling
software on Xenix 286 ages ago (e.g. 0 instead of NULL passed as the
last argument to exec).  While that is a fascinating excercise, it's
not what happens here.


-uwe