On Fri, 09 May 2014 15:21:51 +0530 Srivardhan <sri.heb...@samsung.com> wrote:
> > > > -----Original Message----- > > From: Pekka Paalanen [mailto:ppaala...@gmail.com] > > Sent: Friday, May 09, 2014 3:09 PM > > To: Srivardhan > > Cc: 'Hardening'; wayland-devel@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.heb...@samsung.com> wrote: > > > > > > > > > > > > -----Original Message----- > > > > From: wayland-devel [mailto:wayland-devel- > > > > boun...@lists.freedesktop.org] On Behalf Of Hardening > > > > Sent: Friday, May 09, 2014 1:08 PM > > > > To: wayland-devel@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.heb...@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 _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel