RE: [PATCH] event: Cheking for NULL before dereferencing the pointer.
-Original Message- From: wayland-devel [mailto:wayland-devel- boun...@lists.freedesktop.org] On Behalf Of Pekka Paalanen Sent: Friday, May 09, 2014 3:50 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 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, Will update the patch with assert and send. Thanks, pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
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. Regards. -- David FORT website: http://www.hardening-consulting.com/ ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
RE: [PATCH] event: Cheking for NULL before dereferencing the pointer.
-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. Thank-you, Hebbar Regards. -- David FORT website: http://www.hardening-consulting.com/ ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
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. Thanks, pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
RE: [PATCH] event: Cheking for NULL before dereferencing the pointer.
-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? Thank-you, Hebbar Thanks, pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] event: Cheking for NULL before dereferencing the pointer.
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
Re: [PATCH] event: Cheking for NULL before dereferencing the pointer.
Le 09/05/2014 12:20, Pekka Paalanen a écrit : 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: [...] 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. IMHO wl_event_source_remove() should take a wl_event_source ** as parameter and set to NULL the event_source pointer (preventing anyone to use it). Using eclipse call hierarchy, I've seen many places where this extra precaution is not taken. I don't know if wl_event_source_remove() can be considered as part of the libwayland API and so fixed in stone ? Regards. -- David FORT website: http://www.hardening-consulting.com/ ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] event: Cheking for NULL before dereferencing the pointer.
On Fri, 09 May 2014 14:50:19 +0200 Hardening rdp.eff...@gmail.com wrote: Le 09/05/2014 12:20, Pekka Paalanen a écrit : 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: [...] 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. IMHO wl_event_source_remove() should take a wl_event_source ** as parameter and set to NULL the event_source pointer (preventing anyone to use it). Using eclipse call hierarchy, I've seen many places where this extra precaution is not taken. I don't know if wl_event_source_remove() can be considered as part of the libwayland API and so fixed in stone ? If it is exported in a release, it is set in stone. And so it is. Thanks, pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] event: Cheking for NULL before dereferencing the pointer.
On Fri, May 09, 2014 at 02:56:14PM +0530, Srivardhan 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. 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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel