[PATCH weston 3/3] xwm: Test hash_table_lookup() returns

[Pekka Paalanen]

On Tue,  7 Apr 2015 12:12:15 -0500
Derek Foreman <derekf at osg.samsung.com> wrote:

> Make sure we always test hash_table_lookup()s return to prevent trying to
> dereference a NULL window.
> 
> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
> ---
>  xwayland/window-manager.c | 103 +++++++++++++++++++++++++++++++++-------------
>  1 file changed, 74 insertions(+), 29 deletions(-)
> 
> diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c
> index 3967670..9388d2e 100644
> --- a/xwayland/window-manager.c
> +++ b/xwayland/window-manager.c
> @@ -446,10 +446,16 @@ weston_wm_window_read_properties(struct weston_wm_window *window)
>  				strndup(xcb_get_property_value(reply),
>  					xcb_get_property_value_length(reply));
>  			break;
> -		case XCB_ATOM_WINDOW:
> +		case XCB_ATOM_WINDOW: {
> +			int found;
>  			xid = xcb_get_property_value(reply);
> -			hash_table_lookup(wm->window_hash, *xid, (struct weston_wm_window **)p);
> +			found = hash_table_lookup(wm->window_hash, *xid,
> +					      (struct weston_wm_window **)p);
> +			if (!found)
> +				weston_log("XCB_ATOM_WINDOW contains window"
> +					   " id not found in hash table.\n");
>  			break;
> +		}
>  		case XCB_ATOM_CARDINAL:
>  		case XCB_ATOM_ATOM:
>  			atom = xcb_get_property_value(reply);
> @@ -586,14 +592,18 @@ weston_wm_handle_configure_request(struct weston_wm *wm, xcb_generic_event_t *ev
>  		(xcb_configure_request_event_t *) event;
>  	struct weston_wm_window *window;
>  	uint32_t mask, values[16];
> -	int x, y, width, height, i = 0;
> +	int x, y, width, height, i = 0, found;
>  
>  	wm_log("XCB_CONFIGURE_REQUEST (window %d) %d,%d @ %dx%d\n",
>  	       configure_request->window,
>  	       configure_request->x, configure_request->y,
>  	       configure_request->width, configure_request->height);
>  
> -	hash_table_lookup(wm->window_hash, configure_request->window, &window);
> +	found = hash_table_lookup(wm->window_hash,
> +				  configure_request->window,
> +				  &window);
> +	if (!found)
> +		return;
>  
>  	if (window->fullscreen) {
>  		weston_wm_window_send_configure_notify(window);
> @@ -650,6 +660,8 @@ our_resource(struct weston_wm *wm, uint32_t id)
>  static void
>  weston_wm_handle_configure_notify(struct weston_wm *wm, xcb_generic_event_t *event)
>  {
> +	int found;
> +
>  	xcb_configure_notify_event_t *configure_notify = 
>  		(xcb_configure_notify_event_t *) event;
>  	struct weston_wm_window *window;
> @@ -659,7 +671,12 @@ weston_wm_handle_configure_notify(struct weston_wm *wm, xcb_generic_event_t *eve
>  	       configure_notify->x, configure_notify->y,
>  	       configure_notify->width, configure_notify->height);
>  
> -	hash_table_lookup(wm->window_hash, configure_notify->window, &window);
> +	found = hash_table_lookup(wm->window_hash,
> +				  configure_notify->window,
> +				  &window);
> +	if (!found)
> +		return;

Hi,

on a quick look this whole patch seems fine, I'll read it properly the
next time. :-)

Would it save anything to have a helper:

bool
wm_lookup_window(struct weston_wm *wm, uint32_t xid,
		 struct weston_wm_window **ret);

or something like that? We're always passing wm->window_hash anyway.
Letting you write
	found = wm_lookup_window(wm, blargh, &window);
which is a bit shorter.

That way you wouldn't need to change the pointer type for
hash_table_lookup() either, leaving it generic if we happen to need a
hash of something else. Hm?


Thanks,
pq