Re: CVS commit: src/sys/kern

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

On Thu, Nov 07, 2019 at 15:48:55 +0300, Valery Ushakov wrote:

> 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.

Lest the legalese part of this sub-thread distract from this point let
me re-iterate.  I'm not questioning primarily whether that
&dlp->d_magic is UB or not (it most likely is).  What irks me about
this change is that we tweak one random instance of a larger problem
and disregard the rest of it.

If I misunderstand the problem and other uses of dpl->foo in the same
file are ok, please, kindly point this out to me.

If the other uses are indeed problematic, then does or doesn't the
sanitizer complain about them, like it complains about that check that
was "fixed" in the original commit?

If the sanitizer does complain about other uses, there is little point
in fixing one instance and not the others.

If the sanitizer does NOT complain about other uses, then please find
a different way to shup the stupid thing up.  (Which is also how I
read the responses from Martin and Christos, but that's just my
interpretation).

-uwe