On 08/04/15 07:41 AM, Pekka Paalanen wrote: > On Tue, 7 Apr 2015 12:12:15 -0500 > Derek Foreman <[email protected]> wrote: > >> Make sure we always test hash_table_lookup()s return to prevent trying to >> dereference a NULL window. >> >> Signed-off-by: Derek Foreman <[email protected]> >> --- >> 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 > _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
