[PATCH wayland 3/3] shm: wl_shm_buffer_get_data() requires a valid pool.

[Derek Foreman]

On 16/07/15 03:02 AM, Hardening wrote:
> Le 01/07/2015 12:52, Marek Chalupa a écrit :
>>
>>
>> On Wed, Jul 1, 2015 at 11:25 AM, Marek Chalupa <mchqwerty at gmail.com
>> <mailto:mchqwerty at gmail.com>> wrote:
>>
>>
>>
>>     On Fri, Jun 26, 2015 at 6:35 PM, Derek Foreman
>>     <derekf at osg.samsung.com <mailto:derekf at osg.samsung.com>> wrote:
>>
>>         There's no situation where a shm buffer without a pool makes sense,
>>         so we enforce the pool's existence a little more rigidly.
>>
>>         Signed-off-by: Derek Foreman <derekf at osg.samsung.com
>>         <mailto:derekf at osg.samsung.com>>
>>         ---
>>          src/wayland-shm.c | 10 ++++++----
>>          1 file changed, 6 insertions(+), 4 deletions(-)
>>
>>         diff --git a/src/wayland-shm.c b/src/wayland-shm.c
>>         index da11743..79b3886 100644
>>         --- a/src/wayland-shm.c
>>         +++ b/src/wayland-shm.c
>>         @@ -353,10 +353,12 @@ wl_shm_buffer_get_stride(struct
>>         wl_shm_buffer *buffer)
>>          WL_EXPORT void *
>>          wl_shm_buffer_get_data(struct wl_shm_buffer *buffer)
>>          {
>>         -       if (buffer->pool)
>>         -               return buffer->pool->data + buffer->offset;
>>         -       else
>>         -               return buffer + 1;
>>         +       assert(buffer->pool);
>>         +
>>         +       if (!buffer->pool)
>>         +               return NULL;
>>
>>
>>     This condition is never true if the assert passed
>>
>>  
>> Yeah, but in non-debug build we do not have the assert, so it may make
>> sense to have this doubled. Ok.
>>  
> 
> I find it a little strange, that something that abort() in debug mode,
> just "silently" return NULL in production mode...
> 
> Checking with the Eclipse "Call hierarchy" tool, none of the callers of
> wl_shm_buffer_get_data() check the return value, so we may detect the
> bug quite far from the real source.
> 
> So my suggestion is either wl_shm_buffer_get_data is a "succeed or kill
> program" function (like xyzalloc functions), or the callers have to be
> updated if we can live with buffer->pool == NULL.
> 
> Best regards.


I'm wondering if buffer->pool can ever actually be NULL anyway...  I
have a suspicion that it can't, making the assert and the test a little
silly.