egl_dri2: check if driver_name is NULL before releasing it

Submitted by Zhaowei Yuan on Oct. 30, 2018, 6:26 a.m.

Details

Message ID 1540880807-15511-1-git-send-email-zhaowei.yuan@samsung.com
State New
Headers show
Series "egl_dri2: check if driver_name is NULL before releasing it" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Zhaowei Yuan Oct. 30, 2018, 6:26 a.m.
Pointer dri2_dpy->driver_name is probably NULL when calling
dri2_display_destory, check this before releasing it.

Signed-off-by: Zhaowei Yuan <zhaowei.yuan@samsung.com>
---
 src/egl/drivers/dri2/egl_dri2.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index c5fa935..54cc334 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -967,7 +967,8 @@  dri2_display_destroy(_EGLDisplay *disp)
       close(dri2_dpy->fd);
    if (dri2_dpy->driver)
       dlclose(dri2_dpy->driver);
-   free(dri2_dpy->driver_name);
+   if (dri2_dpy->driver_name)
+      free(dri2_dpy->driver_name);
 
 #ifdef HAVE_WAYLAND_PLATFORM
    free(dri2_dpy->device_name);

Comments

On 10/30/18 8:26 AM, Zhaowei Yuan wrote:
> Pointer dri2_dpy->driver_name is probably NULL when calling
> dri2_display_destory, check this before releasing it.

It's fine for it to be NULL though and it looks like all the drivers set 
it correctly, there is no need for such check. Does this change fix 
something for you, what was the motivation for this change?

(same applies for the device_name patch)

> Signed-off-by: Zhaowei Yuan <zhaowei.yuan@samsung.com>
> ---
>   src/egl/drivers/dri2/egl_dri2.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index c5fa935..54cc334 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -967,7 +967,8 @@ dri2_display_destroy(_EGLDisplay *disp)
>         close(dri2_dpy->fd);
>      if (dri2_dpy->driver)
>         dlclose(dri2_dpy->driver);
> -   free(dri2_dpy->driver_name);
> +   if (dri2_dpy->driver_name)
> +      free(dri2_dpy->driver_name);
>   
>   #ifdef HAVE_WAYLAND_PLATFORM
>      free(dri2_dpy->device_name);
>
I don't think it's fine, usually, freeing an NULL pointer will cause 
unexpected errors. It's better to check this for the robustness.

On 10/30/18 3:17 PM, Tapani Pälli wrote:
> On 10/30/18 8:26 AM, Zhaowei Yuan wrote:
>> Pointer dri2_dpy->driver_name is probably NULL when calling
>> dri2_display_destory, check this before releasing it.
> 
> It's fine for it to be NULL though and it looks like all the drivers set 
> it correctly, there is no need for such check. Does this change fix 
> something for you, what was the motivation for this change?
> 
> (same applies for the device_name patch)
> 
>> Signed-off-by: Zhaowei Yuan <zhaowei.yuan@samsung.com>
>> ---
>>   src/egl/drivers/dri2/egl_dri2.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/egl/drivers/dri2/egl_dri2.c 
>> b/src/egl/drivers/dri2/egl_dri2.c
>> index c5fa935..54cc334 100644
>> --- a/src/egl/drivers/dri2/egl_dri2.c
>> +++ b/src/egl/drivers/dri2/egl_dri2.c
>> @@ -967,7 +967,8 @@ dri2_display_destroy(_EGLDisplay *disp)
>>         close(dri2_dpy->fd);
>>      if (dri2_dpy->driver)
>>         dlclose(dri2_dpy->driver);
>> -   free(dri2_dpy->driver_name);
>> +   if (dri2_dpy->driver_name)
>> +      free(dri2_dpy->driver_name);
>>   #ifdef HAVE_WAYLAND_PLATFORM
>>      free(dri2_dpy->device_name);
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Tue, 2018-10-30 at 18:28 +0800, Zhaowei YUan wrote:
> I don't think it's fine, usually, freeing an NULL pointer will cause 
> unexpected errors. It's better to check this for the robustness.
> 

From http://pubs.opengroup.org/onlinepubs/009695399/functions/free.html
:

"If ptr is a null pointer, no action shall occur."

So, it should be fine. If your system free does something when passed a
null-pointer, it sounds like there's a problem with your system. This
assumption is made all over the place, not just in Mesa.

> On 10/30/18 3:17 PM, Tapani Pälli wrote:
> > On 10/30/18 8:26 AM, Zhaowei Yuan wrote:
> > > Pointer dri2_dpy->driver_name is probably NULL when calling
> > > dri2_display_destory, check this before releasing it.
> > 
> > It's fine for it to be NULL though and it looks like all the
> > drivers set 
> > it correctly, there is no need for such check. Does this change
> > fix 
> > something for you, what was the motivation for this change?
> > 
> > (same applies for the device_name patch)
> > 
> > > Signed-off-by: Zhaowei Yuan <zhaowei.yuan@samsung.com>
> > > ---
> > >   src/egl/drivers/dri2/egl_dri2.c | 3 ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/egl/drivers/dri2/egl_dri2.c 
> > > b/src/egl/drivers/dri2/egl_dri2.c
> > > index c5fa935..54cc334 100644
> > > --- a/src/egl/drivers/dri2/egl_dri2.c
> > > +++ b/src/egl/drivers/dri2/egl_dri2.c
> > > @@ -967,7 +967,8 @@ dri2_display_destroy(_EGLDisplay *disp)
> > >         close(dri2_dpy->fd);
> > >      if (dri2_dpy->driver)
> > >         dlclose(dri2_dpy->driver);
> > > -   free(dri2_dpy->driver_name);
> > > +   if (dri2_dpy->driver_name)
> > > +      free(dri2_dpy->driver_name);
> > >   #ifdef HAVE_WAYLAND_PLATFORM
> > >      free(dri2_dpy->device_name);
> > > 
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev