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