Issue 5498090: Fix ibus-x11 SEGV in _process_key_event_done. (Closed)
|
Created:
12 years, 6 months ago by fujiwara Modified:
12 years, 2 months ago Reviewers:
shawn.p.huang CC:
shawn.p.huang_gmail.com Base URL:
git://github.com/ibus/ibus.git@master Visibility:
Public. |
DescriptionFix ibus-x11 SEGV in _process_key_event_done.
IMForwardEvent() calls _Xi18nFindClient() and it could return NULL.
Maybe the connect_id would be disconnected during the async
process_key_event.
This fix checks XIM_DISCONNECT in ims_protocol_handler() to cancel
IMForwardEvent() in _process_key_event_done().
BUG=RH#769135
TEST=Linux desktop
Patch Set 1 #
Total comments: 3
Patch Set 2 : Updated patch to follow Message #2 #
Total comments: 2
Patch Set 3 : Removed xim_connect_ic #
Total comments: 1
Patch Set 4 : Updated with message #10. #Patch Set 5 : Updated with the latest master. #MessagesTotal messages: 12
http://codereview.appspot.com/5498090/diff/1/client/x11/main.c File client/x11/main.c (right): http://codereview.appspot.com/5498090/diff/1/client/x11/main.c#newcode469 client/x11/main.c:469: if (pfe->connect_id == disconnected_id) { Instead of checking with disconnected_id, we could check if the connect_id and ic_id is in the connections table ic table. if not, we could just discard the event. http://codereview.appspot.com/5498090/diff/1/client/x11/main.c#newcode575 client/x11/main.c:575: g_hash_table_insert (_connections, I found there is a XIM_CONNECT message, if this message will be sent to server when a new connection is come in? If it is true, probably we could create the X11ICONN instance with XIM_CONNECT message? http://codereview.appspot.com/5498090/diff/1/client/x11/main.c#newcode748 client/x11/main.c:748: disconnected_id = call_data->connect_id; I think we should remove connect_id from _connections table, and all related ics from _x11_ic_table.
Sign in to reply to this message.
I revised the patch. Thanks. On 2011/12/30 15:33:38, Peng wrote: > http://codereview.appspot.com/5498090/diff/1/client/x11/main.c > File client/x11/main.c (right): > > http://codereview.appspot.com/5498090/diff/1/client/x11/main.c#newcode469 > client/x11/main.c:469: if (pfe->connect_id == disconnected_id) { > Instead of checking with disconnected_id, we could check if the connect_id and > ic_id is in the connections table ic table. if not, we could just discard the > event. > > http://codereview.appspot.com/5498090/diff/1/client/x11/main.c#newcode575 > client/x11/main.c:575: g_hash_table_insert (_connections, > I found there is a XIM_CONNECT message, if this message will be sent to server > when a new connection is come in? > > If it is true, probably we could create the X11ICONN instance with XIM_CONNECT > message? > > http://codereview.appspot.com/5498090/diff/1/client/x11/main.c#newcode748 > client/x11/main.c:748: disconnected_id = call_data->connect_id; > I think we should remove connect_id from _connections table, and all related ics > from _x11_ic_table.
Sign in to reply to this message.
http://codereview.appspot.com/5498090/diff/4001/client/x11/main.c File client/x11/main.c (right): http://codereview.appspot.com/5498090/diff/4001/client/x11/main.c#newcode574 client/x11/main.c:574: g_hash_table_insert (_connections, Should we create the conn in xim_open or xim_connect? Could you double check it? I believe we should only do it in one place. If it is necessary to create conn in both, probably we should remove g_return_val_if_fail (conn == NULL, 0). http://codereview.appspot.com/5498090/diff/4001/client/x11/main.c#newcode672 client/x11/main.c:672: g_list_free (conn->clients); You may use g_list_free_full(conn->clients, _free_ic)
Sign in to reply to this message.
On 2012/01/03 15:27:44, Peng wrote: > http://codereview.appspot.com/5498090/diff/4001/client/x11/main.c > File client/x11/main.c (right): > > http://codereview.appspot.com/5498090/diff/4001/client/x11/main.c#newcode574 > client/x11/main.c:574: g_hash_table_insert (_connections, > Should we create the conn in xim_open or xim_connect? Could you double check it? > I believe we should only do it in one place. When I invoke xterm, xim_open only is called but no xim_connect/disconnect. Maybe we don't need xim_connect/disconnect . > > If it is necessary to create conn in both, probably we should remove > g_return_val_if_fail (conn == NULL, 0). > > http://codereview.appspot.com/5498090/diff/4001/client/x11/main.c#newcode672 > client/x11/main.c:672: g_list_free (conn->clients); > You may use g_list_free_full(conn->clients, _free_ic)
Sign in to reply to this message.
On 2012/01/04 06:02:49, fujiwara wrote: > On 2012/01/03 15:27:44, Peng wrote: > > http://codereview.appspot.com/5498090/diff/4001/client/x11/main.c > > File client/x11/main.c (right): > > > > http://codereview.appspot.com/5498090/diff/4001/client/x11/main.c#newcode574 > > client/x11/main.c:574: g_hash_table_insert (_connections, > > Should we create the conn in xim_open or xim_connect? Could you double check > it? > > I believe we should only do it in one place. > > When I invoke xterm, xim_open only is called but no xim_connect/disconnect. > > Maybe we don't need xim_connect/disconnect . You mean this CL is not necessary anymore? > > > > > If it is necessary to create conn in both, probably we should remove > > g_return_val_if_fail (conn == NULL, 0). > > > > http://codereview.appspot.com/5498090/diff/4001/client/x11/main.c#newcode672 > > client/x11/main.c:672: g_list_free (conn->clients); > > You may use g_list_free_full(conn->clients, _free_ic)
Sign in to reply to this message.
On 2012/01/04 15:25:31, Peng wrote: > On 2012/01/04 06:02:49, fujiwara wrote: > > On 2012/01/03 15:27:44, Peng wrote: > > > http://codereview.appspot.com/5498090/diff/4001/client/x11/main.c > > > File client/x11/main.c (right): > > > > > > http://codereview.appspot.com/5498090/diff/4001/client/x11/main.c#newcode574 > > > client/x11/main.c:574: g_hash_table_insert (_connections, > > > Should we create the conn in xim_open or xim_connect? Could you double check > > it? > > > I believe we should only do it in one place. > > > > When I invoke xterm, xim_open only is called but no xim_connect/disconnect. > > > > Maybe we don't need xim_connect/disconnect . > > You mean this CL is not necessary anymore? Hmm.., my understanding is XIM_DISCONNECT only calls _Xi18nDeleteClient() from _Xi18nMessageHandler() and XIM_CLOSE does not release connect_id. But neither XIM_DISCONNECT nor XIM_CLOSE is called in my test. Personally I'd think a risk to remove g_hash_table_remove() in xim_close() and it might be good to keep g_hash_table_remove() in both xim_close() and xim_disconnect_ic(). _Xi18nCheckXAddress() assigns Xi18nXDisconnect(). > > > > > > > > > If it is necessary to create conn in both, probably we should remove > > > g_return_val_if_fail (conn == NULL, 0). > > > > > > http://codereview.appspot.com/5498090/diff/4001/client/x11/main.c#newcode672 > > > client/x11/main.c:672: g_list_free (conn->clients); > > > You may use g_list_free_full(conn->clients, _free_ic)
Sign in to reply to this message.
On 2012/01/05 02:30:42, fujiwara wrote: > On 2012/01/04 15:25:31, Peng wrote: > > On 2012/01/04 06:02:49, fujiwara wrote: > > > On 2012/01/03 15:27:44, Peng wrote: > > > > http://codereview.appspot.com/5498090/diff/4001/client/x11/main.c > > > > File client/x11/main.c (right): > > > > > > > > > http://codereview.appspot.com/5498090/diff/4001/client/x11/main.c#newcode574 > > > > client/x11/main.c:574: g_hash_table_insert (_connections, > > > > Should we create the conn in xim_open or xim_connect? Could you double > check > > > it? > > > > I believe we should only do it in one place. > > > > > > When I invoke xterm, xim_open only is called but no xim_connect/disconnect. > > > > > > Maybe we don't need xim_connect/disconnect . > > > > You mean this CL is not necessary anymore? > > > Hmm.., my understanding is XIM_DISCONNECT only calls _Xi18nDeleteClient() from > _Xi18nMessageHandler() and XIM_CLOSE does not release connect_id. > But neither XIM_DISCONNECT nor XIM_CLOSE is called in my test. > Personally I'd think a risk to remove g_hash_table_remove() in xim_close() and > it might be good to keep g_hash_table_remove() in both xim_close() and > xim_disconnect_ic(). > _Xi18nCheckXAddress() assigns Xi18nXDisconnect(). > > > > > > > > > > > > > > If it is necessary to create conn in both, probably we should remove > > > > g_return_val_if_fail (conn == NULL, 0). > > > > > > > > > http://codereview.appspot.com/5498090/diff/4001/client/x11/main.c#newcode672 > > > > client/x11/main.c:672: g_list_free (conn->clients); > > > > You may use g_list_free_full(conn->clients, _free_ic) Is this CL still necessary? If not, please close it.
Sign in to reply to this message.
On 2012/02/29 15:41:49, Peng wrote: > On 2012/01/05 02:30:42, fujiwara wrote: > > On 2012/01/04 15:25:31, Peng wrote: > > > On 2012/01/04 06:02:49, fujiwara wrote: > > > > On 2012/01/03 15:27:44, Peng wrote: > > > > > http://codereview.appspot.com/5498090/diff/4001/client/x11/main.c > > > > > File client/x11/main.c (right): > > > > > > > > > > > > http://codereview.appspot.com/5498090/diff/4001/client/x11/main.c#newcode574 > > > > > client/x11/main.c:574: g_hash_table_insert (_connections, > > > > > Should we create the conn in xim_open or xim_connect? Could you double > > check > > > > it? > > > > > I believe we should only do it in one place. > > > > > > > > When I invoke xterm, xim_open only is called but no > xim_connect/disconnect. > > > > > > > > Maybe we don't need xim_connect/disconnect . > > > > > > You mean this CL is not necessary anymore? > > > > > > Hmm.., my understanding is XIM_DISCONNECT only calls _Xi18nDeleteClient() from > > _Xi18nMessageHandler() and XIM_CLOSE does not release connect_id. > > But neither XIM_DISCONNECT nor XIM_CLOSE is called in my test. > > Personally I'd think a risk to remove g_hash_table_remove() in xim_close() and > > it might be good to keep g_hash_table_remove() in both xim_close() and > > xim_disconnect_ic(). > > _Xi18nCheckXAddress() assigns Xi18nXDisconnect(). > > > > > > > > > > > > > > > > > > > If it is necessary to create conn in both, probably we should remove > > > > > g_return_val_if_fail (conn == NULL, 0). > > > > > > > > > > > > http://codereview.appspot.com/5498090/diff/4001/client/x11/main.c#newcode672 > > > > > client/x11/main.c:672: g_list_free (conn->clients); > > > > > You may use g_list_free_full(conn->clients, _free_ic) > > Is this CL still necessary? If not, please close it. I removed xim_connect_ic() since it's not cleared if the function is called.
Sign in to reply to this message.
https://codereview.appspot.com/5498090/diff/10001/client/x11/main.c File client/x11/main.c (right): https://codereview.appspot.com/5498090/diff/10001/client/x11/main.c#newcode635 client/x11/main.c:635: xim_disconnect_ic (XIMS xims, IMDisConnectStruct *call_data) The code is almost same with ximclose. Could please try to remove duplicate code by adding a new function?
Sign in to reply to this message.
On 2012/04/04 17:10:51, Peng wrote: > https://codereview.appspot.com/5498090/diff/10001/client/x11/main.c > File client/x11/main.c (right): > > https://codereview.appspot.com/5498090/diff/10001/client/x11/main.c#newcode635 > client/x11/main.c:635: xim_disconnect_ic (XIMS xims, IMDisConnectStruct > *call_data) > The code is almost same with ximclose. Could please try to remove duplicate code > by adding a new function? Updated the patch.
Sign in to reply to this message.
|
