[PATCH] event: Cheking for NULL before dereferencing the pointer.

[Kristian Høgsberg]

On Fri, May 09, 2014 at 02:56:14PM +0530, Srivardhan wrote:
> 
> 
> > -----Original Message-----
> > From: wayland-devel [mailto:wayland-devel-
> > bounces at lists.freedesktop.org] On Behalf Of Hardening
> > Sent: Friday, May 09, 2014 1:08 PM
> > To: wayland-devel at lists.freedesktop.org
> > Subject: Re: [PATCH] event: Cheking for NULL before dereferencing the
> > pointer.
> > 
> > Le 09/05/2014 08:43, Srivardhan Hebbar a écrit :
> > > Checking for NULL before dereferencing the wl_event_source pointer so
> > > as to avoid crash.
> > >
> > > Signed-off-by: Srivardhan Hebbar <sri.hebbar at samsung.com>
> > > ---
> > >   src/event-loop.c |    7 ++++++-
> > >   1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/event-loop.c b/src/event-loop.c index
> > > 9790cde..b62d16e 100644
> > > --- a/src/event-loop.c
> > > +++ b/src/event-loop.c
> > > @@ -312,7 +312,12 @@ wl_event_source_check(struct wl_event_source
> > *source)
> > >   WL_EXPORT int
> > >   wl_event_source_remove(struct wl_event_source *source)
> > >   {
> > > -	struct wl_event_loop *loop = source->loop;
> > > +	struct wl_event_loop *loop;
> > > +
> > > +	if (source == NULL)
> > > +		return 0;
> > > +
> > > +	loop = source->loop;
> > >
> > >   	/* We need to explicitly remove the fd, since closing the fd
> > >   	 * isn't enough in case we've dup'ed the fd. */
> > >
> > 
> > Hello Srivardhan,
> > 
> > do you have a case where this check is hit ? I may be wrong but having no
> > loop associated with a source event is not supposed to happen. So my guess
> > is that a caller of wl_event_source_remove has forgotten to nullify the
> event
> > source after the call.
> 
> Hi,
> 
> I was going through the code of Weston. In the main function in compositor.c
> wl_event_loop_add_signal() is called to allocate the memory for
> signals[](Line no: 4196. struct wl_event_source *signals[4]). The function
> returns NULL if memory allocation failed. After that there is no NULL check
> for 'signals'. In the end while clearing up, this function is called. So if
> memory allocation failed then a NULL is passed to this function. Hence
> adding code to check for the NULL.

You caught a problem here, but the issue is that we don't check for NULL
where we allocate the source.  Passing a NULL pointer to
wl_event_source_remove() is a programmer error and we don't want to
silently fail.

Kristian


> Thank-you,
> Hebbar
> > 
> > Regards.
> > 
> > --
> > David FORT
> > website: http://www.hardening-consulting.com/
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel