Re: CVS commit: src/sys/kern

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

On Thu, Nov 07, 2019 at 13:37:21 +0100, Kamil Rytarowski wrote:

> On 07.11.2019 13:17, Valery Ushakov wrote:
> > On Thu, Nov 07, 2019 at 06:02:39 +0100, Kamil Rytarowski wrote:
> > 
> >> I have checked received the following patch and received a feedback from
> >> a LLVM developer.
> >>
> >> On 07.11.2019 05:47, 'Dmitry Vyukov' via syzkaller-netbsd-bugs wrote:
> >>> I've consulted with some people and _presumably_ (to the degree one
> >>> can be sure about bitter corner cases of C/C++ :)) this is a correct
> >>> fix (and formally correct warnings from ubsan).
> >>> As 6.5.3.2/4 says, only &*p and &p[i] syntactic forms are defined as
> >>> special case of not being a dereference, but rather effectively an
> >>> address calculation. But &p->m is not. Thus it is interpreted as a
> >>> dereference that produces an lvalue and then taking address of that
> >>> lvalue. At the point of dereference we have UB. Your fix avoids the
> >>> dereference.
> > 
> > The context is lost in the thread, but the original change was about
> > &dlp->d_magic as far as I can figure out.  If the claim is that that's
> > UB b/c dlp is improperly aligned, then why the half of the rest of the
> > file is not UB as it uses the same "dlp" pointer to access other
> > members of the disklabel.
> 
> We were already addressing various reports for disklabel related code in
> the kernel and userland. In userland we as far as I recall just copy the
> struct into an aligned promptly pointer.
> 
> There might be more problems, but we address them as they pop up.

That seems counterintuitive.  There's the root cause and when/if that
root cause is fixed, then this particular problem will be fixed as
well.  The concern obviously is that when the root problem is fixed,
this change will be forgotten and the unnecessarily uglified code will
be just left over as it currently is.

So the change is extremely questionable from that point of view.  It
creates the illusion of things being "fixed" while in the longer run
I'd say it harms the code.  And as I said, if the sanitizer flags that
place as UB and doesn't flag dozens of dlp->foo accesses elsewhere in
that file, then either I don't understand what the sanitizer complains
about, or it's not a very good sanitizer and we shouldn't care to shut
it up in this random place that triggers it.


> > As a side note - the C99 standard contains "derefer" exactly once, in
> > a footnote.  Since we have ended up in the darkest corners of
> > legalistic exegesis, please, can we avoid using the word that is,
> > technically speaking, meaningless as far as this discussion is
> > concerned?
> 
> Unary * oprator. C++ specified term "dereferenceable" in the context of
> the unary * operator.

This is C code and the C standard is hard enough as it is already.
Please, can we put the C++ aside for a moment?


-uwe