[Pidgin] #14392: get_iter_from_chatbuddy can dereference NULL pointer (was: pidgin_conv_chat_rename_user can dereference NULL pointer)

[Pidgin]

#14392: get_iter_from_chatbuddy can dereference NULL pointer
------------------------------------------+---------------------------------
 Reporter:  clh                           |        Owner:              
     Type:  defect                        |       Status:  new         
Milestone:                                |    Component:  pidgin (gtk)
  Version:  2.9.0                         |   Resolution:              
 Keywords:  get_iter_from_chatbuddy NULL  |  
------------------------------------------+---------------------------------
Description changed by clh:

Old description:

> If we look at pidgin_conv_chat_rename_user():
> {{{
> ...
>         old_cbuddy = purple_conv_chat_cb_find(chat, old_name);
>         if (get_iter_from_chatbuddy(old_cbuddy, &iter)) {
> ...
>         }
> ...
>         if (!old_cbuddy)
>                 return;
> ...
> }}}
>
> We see that purple_conv_chat_cb_find() can return NULL, there is even a
> check for it. However, before the check we use the return as argument for
> get_iter_from_chatbuddy() which will dereference the pointer without
> checking for NULL:
>
> {{{
>  static gboolean get_iter_from_chatbuddy(PurpleConvChatBuddy *cb,
> GtkTreeIter *iter)
> {
>         GtkTreeRowReference *ref = cb->ui_data;
> }}}
>
> The same happens in pidgin_conv_chat_update_user().
>
> My suggested fix would be checking the argument in
> get_iter_from_chatbuddy():
>
> {{{
>  static gboolean get_iter_from_chatbuddy(PurpleConvChatBuddy *cb,
> GtkTreeIter *iter)
>  {
> -       GtkTreeRowReference *ref = cb->ui_data;
> +       GtkTreeRowReference *ref;
>         GtkTreePath *path;
>         GtkTreeModel *model;
>
> +       if (!cb)
> +               return FALSE;
> +
> +       ref = cb->ui_data;
> +
>         if (!ref)
>                 return FALSE;
>
> }}}

New description:

 If we look at pidgin_conv_chat_rename_user():
 {{{
 ...
         old_cbuddy = purple_conv_chat_cb_find(chat, old_name);
         if (get_iter_from_chatbuddy(old_cbuddy, &iter)) {
 ...
         }
 ...
         if (!old_cbuddy)
                 return;
 ...
 }}}

 We see that purple_conv_chat_cb_find() can return NULL, there is even a
 check for it. However, before the check we use the return as argument for
 get_iter_from_chatbuddy() which will dereference the pointer without
 checking for NULL:

 {{{
  static gboolean get_iter_from_chatbuddy(PurpleConvChatBuddy *cb,
 GtkTreeIter *iter)
 {
         GtkTreeRowReference *ref = cb->ui_data;
 }}}

 The same happens in pidgin_conv_chat_update_user().

 My suggested fix would be checking the argument in
 get_iter_from_chatbuddy():

 {{{
  static gboolean get_iter_from_chatbuddy(PurpleConvChatBuddy *cb,
 GtkTreeIter *iter)
  {
 -       GtkTreeRowReference *ref = cb->ui_data;
 +       GtkTreeRowReference *ref;
         GtkTreePath *path;
         GtkTreeModel *model;

 +       if (!cb)
 +               return FALSE;
 +
 +       ref = cb->ui_data;
 +
         if (!ref)
                 return FALSE;
 }}}

 This was introduced in 2.9.0, as the code didn't exist before.

--

-- 
Ticket URL: <http://developer.pidgin.im/ticket/14392#comment:2>
Pidgin <http://pidgin.im>
Pidgin