Source-Changes-D archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: CVS commit: src/sys/kern



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.

There is another early crash on boot and I will fix it.

[   1.5079810] panic: UBSan: Undefined Behavior in
/syzkaller/managers/netbsd-kubsan/kernel/sys/kern/vfs_subr.c:793:14,
member access within null pointer of type 'struct vnode_impl'


On 07.11.2019 00:03, Kamil Rytarowski wrote:
> On 06.11.2019 23:38, Christos Zoulas wrote:
>> On Nov 6, 11:17pm, n54%gmx.com@localhost (Kamil Rytarowski) wrote:
>> -- Subject: Re: CVS commit: src/sys/kern
>>
>> | Technically, I think that this is a real UB.
>> | 
>> | 6.3.2.3/7
>> | A pointer to an object type may be converted to a pointer to a
>> | different object type. If the resulting pointer is not correctly
>> | aligned for the referenced type, the behavior is undefined.
>>
>> Then you are right. I guess the rationale for the above is that
>> ... pauses to think ... Dereferencing the new object with the
>> different type can fail if the original pointer was unaligned?
>> I don't see how.
>>
> 
> I recall a similar UB in tmux that is simpler to illustrate.
> 
> struct screen			*s = &data->screen;
> 
> /// if data is NULL -> return
> 
> format_add(ft, "selection_present", "%d", s->sel.flag);
> 
> This triggered UBSan as we syntactically dereferenced a NULL pointer,
> which is UB. The intention was to set s = data + offsetof(data, screen)
> and very compiler optimizes it to do the right thing without real
> dereference.
> 
> Similarly from a syntactical point of view we first dereference
> dlp->d_magic, and next ask for its address &(). This is how I understand
> the GCC behavior here.
> 
> This is certainly very sensitive behavior of the sanitzier, but it can
> catch real bugs. It helped to diagnoze at least a single UVM crash (we
> had crash reports from ASan and UBSan).
> 
>> | I agree that this is appeasing the sanitizer.
>>
>> Yes, on that we agree.
>>
>> christos
>>
> 
> [1]
> https://github.com/tmux/tmux/commit/8fb6666f1733ebd4dcb90ba01dbcfc750190c9df
> 


Attachment: signature.asc
Description: OpenPGP digital signature



Home | Main Index | Thread Index | Old Index