[tpm2] tpm2-abrmd / glib question

[tpm2] tpm2-abrmd / glib question

[Jerry Snitselaar]

[-- Attachment #1: Type: text/plain, Size: 757 bytes --]

Looking at some items that coverity scan came up with for 1.1.0. It is
complaining about a null pointer passed as nonnull parameter with the
pthread_mutex_lock call in access_broker_lock. Is it possible for
access_broker_new to fail (g_object_new doesn't allocate an object?)?
Should there be some check after access_broker_new is called?  Looking
at the glib code, it seemed like there was at least a way through the
code to end up at g_malloc which can return null. I haven't finished
digging through the slab allocator code to see what can happen there.
I've read a comment though by Allison Lortie that said g_object_new
will alway return a non-null value, so I'm wondering what is returned
on failure, or if the claim is it will never fail.

Re: [tpm2] tpm2-abrmd / glib question

[flihp]

[-- Attachment #1: Type: text/plain, Size: 2426 bytes --]

Hi Jerry,

On 10/18/2017 09:46 AM, Jerry Snitselaar wrote:
> Looking at some items that coverity scan came up with for 1.1.0. It is
> complaining about a null pointer passed as nonnull parameter with the
> pthread_mutex_lock call in access_broker_lock.

Are you running these scans yourself or are you looking at the output
from the scan.coverity.com service that we automate through travis-ci? I
haven't merged into the coverity_scan branch in a while (since the last
release) so this may be a new thing but last I checked coverity gave us
a clean bill of health.

> Is it possible for
> access_broker_new to fail (g_object_new doesn't allocate an object?)?

According to the GObject docs GOjbect construction cannot fail.
https://developer.gnome.org/gobject/stable/howto-gobject-construction.html

> Should there be some check after access_broker_new is called? Looking> at the glib code, it seemed like there was at least a way through the
> code to end up at g_malloc which can return null. I haven't finished
> digging through the slab allocator code to see what can happen there.
> I've read a comment though by Allison Lortie that said g_object_new
> will alway return a non-null value, so I'm wondering what is returned
> on failure, or if the claim is it will never fail.

This object needs some love. I've been going back and correcting common
GObject mistakes that I've made but there's still a ways to go. I did
object finalization last week: https://github.com/01org/tpm2-abrmd/pull/211

But the way I've coded the function isn't right since the SAPI context
creation in this _new function may fail (a situation currently ignored
by the code). GObject has a mechanism for doing initialization that
could fail after object creation, I just didn't know about it at the
time :( Thankfully Allison Lortie has a great blog post describing the
feature:
https://blogs.gnome.org/desrt/2012/02/26/a-gentle-introduction-to-gobject-construction/

Quick summary of the issue in this object:
1) g_object_new cannot fail, if a memory allocation fails I think GLib
will abort the program. So I think the situation you're looking into
isn't an issue.
2) The AccessBroker _new function needs to be cleaned up a bit to use
the interface described by Allison in the link above (GInitable).

I'll queue up #2 from this list as an issue in the tabrmd github so I
don't forget.

Philip

Re: [tpm2] tpm2-abrmd / glib question

[Jerry Snitselaar]

[-- Attachment #1: Type: text/plain, Size: 3426 bytes --]

On Wed Oct 18 17, flihp wrote:
>Hi Jerry,
>
>On 10/18/2017 09:46 AM, Jerry Snitselaar wrote:
>> Looking at some items that coverity scan came up with for 1.1.0. It is
>> complaining about a null pointer passed as nonnull parameter with the
>> pthread_mutex_lock call in access_broker_lock.
>
>Are you running these scans yourself or are you looking at the output
>from the scan.coverity.com service that we automate through travis-ci? I
>haven't merged into the coverity_scan branch in a while (since the last
>release) so this may be a new thing but last I checked coverity gave us
>a clean bill of health.
>

This is a scan that gets run against our packages here as part of the
release process. As part of getting tpm2-abrmd into RHEL I have to go
through the covscan output it generates, and determine if they are
actual bugs or not. The two I've seen so far that were bugs, you've
already dealt with upstream. The rest have been false positives.

This was the last one I was dealing with, and while I could tell
the program would never get to that point without the broker allocated,
blowing up long before it got there, I wasn't sure about whether or
not it should be checked after allocation.

>> Is it possible for
>> access_broker_new to fail (g_object_new doesn't allocate an object?)?
>
>According to the GObject docs GOjbect construction cannot fail.
>https://developer.gnome.org/gobject/stable/howto-gobject-construction.html
>

Thanks for the link. I've never dealt with glib, and was digging through
stuff, but hadn't got to this page.

>> Should there be some check after access_broker_new is called? Looking> at the glib code, it seemed like there was at least a way through the
>> code to end up at g_malloc which can return null. I haven't finished
>> digging through the slab allocator code to see what can happen there.
>> I've read a comment though by Allison Lortie that said g_object_new
>> will alway return a non-null value, so I'm wondering what is returned
>> on failure, or if the claim is it will never fail.
>
>This object needs some love. I've been going back and correcting common
>GObject mistakes that I've made but there's still a ways to go. I did
>object finalization last week: https://github.com/01org/tpm2-abrmd/pull/211
>
>But the way I've coded the function isn't right since the SAPI context
>creation in this _new function may fail (a situation currently ignored
>by the code). GObject has a mechanism for doing initialization that
>could fail after object creation, I just didn't know about it at the
>time :( Thankfully Allison Lortie has a great blog post describing the
>feature:
>https://blogs.gnome.org/desrt/2012/02/26/a-gentle-introduction-to-gobject-construction/
>

I think it was a comment here where she mentioned that g_object_new would always
return a non-null value.

>Quick summary of the issue in this object:
>1) g_object_new cannot fail, if a memory allocation fails I think GLib
>will abort the program. So I think the situation you're looking into
>isn't an issue.
>2) The AccessBroker _new function needs to be cleaned up a bit to use
>the interface described by Allison in the link above (GInitable).
>
>I'll queue up #2 from this list as an issue in the tabrmd github so I
>don't forget.
>
>Philip

Yeah, I will go ahead and mark this one as not a bug, and finish up
this process. :)

Re: [tpm2] tpm2-abrmd / glib question

[flihp]

[-- Attachment #1: Type: text/plain, Size: 3801 bytes --]

On 10/18/2017 11:33 AM, Jerry Snitselaar wrote:
> On Wed Oct 18 17, flihp wrote:
>> Hi Jerry,
>>
>> On 10/18/2017 09:46 AM, Jerry Snitselaar wrote:
>>> Looking at some items that coverity scan came up with for 1.1.0. It is
>>> complaining about a null pointer passed as nonnull parameter with the
>>> pthread_mutex_lock call in access_broker_lock.
>>
>> Are you running these scans yourself or are you looking at the output
>> from the scan.coverity.com service that we automate through travis-ci? I
>> haven't merged into the coverity_scan branch in a while (since the last
>> release) so this may be a new thing but last I checked coverity gave us
>> a clean bill of health.
>>
> 
> This is a scan that gets run against our packages here as part of the
> release process. As part of getting tpm2-abrmd into RHEL I have to go
> through the covscan output it generates, and determine if they are
> actual bugs or not. The two I've seen so far that were bugs, you've
> already dealt with upstream. The rest have been false positives.
> 
> This was the last one I was dealing with, and while I could tell
> the program would never get to that point without the broker allocated,
> blowing up long before it got there, I wasn't sure about whether or
> not it should be checked after allocation.
> 
>>> Is it possible for
>>> access_broker_new to fail (g_object_new doesn't allocate an object?)?
>>
>> According to the GObject docs GOjbect construction cannot fail.
>> https://developer.gnome.org/gobject/stable/howto-gobject-construction.html
>>
>>
> 
> Thanks for the link. I've never dealt with glib, and was digging through
> stuff, but hadn't got to this page.
> 
>>> Should there be some check after access_broker_new is called?
>>> Looking> at the glib code, it seemed like there was at least a way
>>> through the
>>> code to end up at g_malloc which can return null. I haven't finished
>>> digging through the slab allocator code to see what can happen there.
>>> I've read a comment though by Allison Lortie that said g_object_new
>>> will alway return a non-null value, so I'm wondering what is returned
>>> on failure, or if the claim is it will never fail.
>>
>> This object needs some love. I've been going back and correcting common
>> GObject mistakes that I've made but there's still a ways to go. I did
>> object finalization last week:
>> https://github.com/01org/tpm2-abrmd/pull/211
>>
>> But the way I've coded the function isn't right since the SAPI context
>> creation in this _new function may fail (a situation currently ignored
>> by the code). GObject has a mechanism for doing initialization that
>> could fail after object creation, I just didn't know about it at the
>> time :( Thankfully Allison Lortie has a great blog post describing the
>> feature:
>> https://blogs.gnome.org/desrt/2012/02/26/a-gentle-introduction-to-gobject-construction/
>>
>>
> 
> I think it was a comment here where she mentioned that g_object_new
> would always
> return a non-null value.

I think this is the one you're referring to:
https://blogs.gnome.org/desrt/2012/02/26/a-gentle-introduction-to-gobject-construction/#comment-1233

> 
>> Quick summary of the issue in this object:
>> 1) g_object_new cannot fail, if a memory allocation fails I think GLib
>> will abort the program. So I think the situation you're looking into
>> isn't an issue.
>> 2) The AccessBroker _new function needs to be cleaned up a bit to use
>> the interface described by Allison in the link above (GInitable).
>>
>> I'll queue up #2 from this list as an issue in the tabrmd github so I
>> don't forget.
>>
>> Philip
> 
> Yeah, I will go ahead and mark this one as not a bug, and finish up
> this process. :)

Excellent.

Philip