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

[Srivardhan]

> -----Original Message-----
> From: wayland-devel [mailto:wayland-devel-
> bounces at lists.freedesktop.org] On Behalf Of Pekka Paalanen
> Sent: Friday, May 09, 2014 3:50 PM
> To: Srivardhan
> Cc: 'Hardening'; wayland-devel at lists.freedesktop.org
> Subject: Re: [PATCH] event: Cheking for NULL before dereferencing the
> pointer.
> 
> On Fri, 09 May 2014 15:21:51 +0530
> Srivardhan <sri.hebbar at samsung.com> wrote:
> 
> >
> >
> > > -----Original Message-----
> > > From: Pekka Paalanen [mailto:ppaalanen at gmail.com]
> > > Sent: Friday, May 09, 2014 3:09 PM
> > > To: Srivardhan
> > > Cc: 'Hardening'; wayland-devel at lists.freedesktop.org
> > > Subject: Re: [PATCH] event: Cheking for NULL before dereferencing
> > > the pointer.
> > >
> > > On Fri, 09 May 2014 14:56:14 +0530
> > > Srivardhan <sri.hebbar at samsung.com> 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.
> > >
> > > Hi,
> > >
> > > I think we should be fixing the caller instead of
> > wl_event_source_remove(). I
> > > do not believe NULL is a valid argument for it, so the bug is in the
> > caller.
> > >
> > > If any wl_event_loop_add_signal() in Weston fails, it would be a
> > > reason to exit with an error.
> > >
> >
> > Oh... k... Will fix that and send the patch...
> >
> > In general isn't it fine to check for NULL before dereferencing?
> 
> Checking is one thing, silently hiding bugs is another thing.
> 
> If NULL is a legal input, then of course it needs to be checked.
> 
> If NULL can happen, but is a runtime error, the program needs to be vocal
> about it, e.g. relay the error back to the caller.
> 
> If API specification says NULL is not a valid input, putting an
> assert() would be fine, since violating that is a programmer error in the
caller.
> 
> I think wl_event_source_remove() falls into the last category. All
functions in
> wayland-util.h belong to this category, too.
> 
Thanks pq,

Will update the patch with assert and send.
> 
> Thanks,
> pq
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel