librt: fix NULL pointer dereference (bug 28213)

On 8/9/21 5:55 PM, Никита Попов via Libc-alpha wrote:
> Helper thread frees copied attribute on NOTIFY_REMOVED message received from the OS kernel. Unfortunately, it fails to check whether copied attribute actually exists (data.attr != NULL). This worked earlier because free() checks passed pointer before actually attempting to release corresponding memory. But __pthread_attr_destroy assumes pointer is not NULL. So passing NULL pointer to __pthread_attr_destroy will result in segmentation fault. This scenario is possible if notification->sigev_notify_attributes == NULL (which means default thread attributes should be used).

Thank you, the fix looks good to me.  Do you have a test case to go with it?

Thanks,
Siddhesh
  

* Siddhesh Poyarekar:

> On 8/9/21 5:55 PM, Никита Попов via Libc-alpha wrote:
>> Helper thread frees copied attribute on NOTIFY_REMOVED message received from the OS kernel. Unfortunately, it fails to check whether copied attribute actually exists (data.attr != NULL). This worked earlier because free() checks passed pointer before actually attempting to release corresponding memory. But __pthread_attr_destroy assumes pointer is not NULL. So passing NULL pointer to __pthread_attr_destroy will result in segmentation fault. This scenario is possible if notification->sigev_notify_attributes == NULL (which means default thread attributes should be used).
>
> Thank you, the fix looks good to me.  Do you have a test case to go with it?

Siddhesh, if you are going to push this, please line-wrap the commit
message before doing so.

Thanks,
Florian
  

On 8/9/21 6:51 PM, Siddhesh Poyarekar wrote:
> On 8/9/21 5:55 PM, Никита Попов via Libc-alpha wrote:
>> Helper thread frees copied attribute on NOTIFY_REMOVED message 
>> received from the OS kernel. Unfortunately, it fails to check whether 
>> copied attribute actually exists (data.attr != NULL). This worked 
>> earlier because free() checks passed pointer before actually 
>> attempting to release corresponding memory. But __pthread_attr_destroy 
>> assumes pointer is not NULL. So passing NULL pointer to 
>> __pthread_attr_destroy will result in segmentation fault. This 
>> scenario is possible if notification->sigev_notify_attributes == NULL 
>> (which means default thread attributes should be used).
> 
> Thank you, the fix looks good to me.  Do you have a test case to go with 
> it?

Also, I don't know if you have an FSF copyright assignment, but it's no 
longer necessary.  Please confirm that you're the original author and 
are authorized to contribute this patch by adding a DCO, i.e. add a 
Signed-off-by to indicate that.  See also:

https://developercertificate.org/

Thanks,
Siddhesh
  

Thanks for the feedback. Yes, I confirm that I'm the original author
of this patch. Here is the adjusted version. If necessary, I can write
proof-of-concept and attach it here.

пн, 9 авг. 2021 г. в 18:32, Siddhesh Poyarekar <siddhesh@gotplt.org>:
>
> On 8/9/21 6:51 PM, Siddhesh Poyarekar wrote:
> > On 8/9/21 5:55 PM, Никита Попов via Libc-alpha wrote:
> >> Helper thread frees copied attribute on NOTIFY_REMOVED message
> >> received from the OS kernel. Unfortunately, it fails to check whether
> >> copied attribute actually exists (data.attr != NULL). This worked
> >> earlier because free() checks passed pointer before actually
> >> attempting to release corresponding memory. But __pthread_attr_destroy
> >> assumes pointer is not NULL. So passing NULL pointer to
> >> __pthread_attr_destroy will result in segmentation fault. This
> >> scenario is possible if notification->sigev_notify_attributes == NULL
> >> (which means default thread attributes should be used).
> >
> > Thank you, the fix looks good to me.  Do you have a test case to go with
> > it?
>
> Also, I don't know if you have an FSF copyright assignment, but it's no
> longer necessary.  Please confirm that you're the original author and
> are authorized to contribute this patch by adding a DCO, i.e. add a
> Signed-off-by to indicate that.  See also:
>
> https://developercertificate.org/
>
> Thanks,
> Siddhesh
  

On 8/9/21 7:15 PM, Никита Попов via Libc-alpha wrote:
> Thanks for the feedback. Yes, I confirm that I'm the original author
> of this patch. Here is the adjusted version. If necessary, I can write
> proof-of-concept and attach it here.

If the PoC is deterministic, it would be useful as a test case[1] to 
protect from future regressions.

Thanks,
Siddhesh

[1] https://sourceware.org/glibc/wiki/Testing/Testsuite
  

On 8/9/21 7:15 PM, Никита Попов via Libc-alpha wrote:
> Thanks for the feedback. Yes, I confirm that I'm the original author
> of this patch. Here is the adjusted version. If necessary, I can write
> proof-of-concept and attach it here.

I have pushed your patch now, thanks again.  We can push the test as a 
separate patch if it is deterministic.

Thanks,
Siddhesh