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

[Derek Foreman]

On 08/04/15 07:41 AM, Pekka Paalanen wrote:
> 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?

And the shorter form probably keeps most if statements under 80 cols
without a temp var. :)

Sure, I'll do that today.

> 
> Thanks,
> pq
>