[virt-tools-list] [PATCH virt-viewer v2 4/5] app: Check validity of hotkey value

[Pavel Grunt]

On Tue, 2016-05-31 at 01:04 +0200, Fabiano Fidêncio wrote:
> On Mon, May 30, 2016 at 5:08 PM, Pavel Grunt <pgrunt at redhat.com> wrote:
> > Related: rhbz#1339572
> > ---
> >  src/virt-viewer-app.c | 5 +++++
> >  tests/test-hotkeys.c  | 4 ++++
> >  2 files changed, 9 insertions(+)
> > 
> > diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c
> > index d449f72..f8fba4c 100644
> > --- a/src/virt-viewer-app.c
> > +++ b/src/virt-viewer-app.c
> > @@ -2077,6 +2077,11 @@ virt_viewer_app_set_hotkeys(VirtViewerApp *self,
> > const gchar *hotkeys_str)
> >          gtk_accelerator_parse(accel, &accel_key, &accel_mods);
> >          g_free(accel);
> > 
> > +        if (strlen(key + 1) != 0 && accel_key == 0 && accel_mods == 0) {
> 
> Not your fault, but this key + 1 made me open the code and double
> check what's going on.
> gchar *key = strstr(*hotkey, "=");
> *key = '\0';
> 
you want to separate key from value, so you replace '=' by '\0' ("key\0value").
If you print *hotkey, you get only the first part.

> Why? :-)
> 
> key++; would be way cleaner as we ened up using it here in your patch
> and somewhere below in the same loop.
ok, I will use key++
> 
> Okay, getting back to your patch ... strlen(key + 1) != 0 ... does it
> really happen?

it should always happen.

The condition says - if value_str is not empty and accel_key=0 and accel_key=0
then it means the value could not be parsed as gtk accelerator.

>  what's the case? 

one can use: virt-viewer --hotkeys="toggle-fullscreen=not_an_accelerator"
which is not consider as a bug in the current code. 

> I would like to see it in the commit
> log. IMO, It shouldn't happen at all. We shouldn't receive a string
> that is: "foo=" and if it does and it is provided by oVirt/RHEVM, the
> bug is there not here :-\.

It is not about rhevm, the bug is about informing the user about incorrect usage
of hotkeys - one can have a typo in commandline and it can be hard to figure out
the issue (I know it is a stupid example).

>  Even though, I understand we should treat
> this case ... so I'd go for: if (key == NULL || ++key == NULL)
That way you would not set *key = '\0', and if key is not NULL then key + 1 is
also not NULL.

>  and let
> it reach the g_warn_reached() part of the code that is just above this
> part you touched.
> 
> About accel_key and accel_mode being 0, again, it only will happen if
> the parser fails. So, having the ++key == NULL in the if would save is
> this part of the code.

Ah you mean *(++key) == '\0'

> 
> > +            g_warning("Invalid value '%s' for key '%s'", key + 1, *hotkey);
> > +            continue;
> > +        }
> > +
> >          if (g_str_equal(*hotkey, "toggle-fullscreen")) {
> >              gtk_accel_map_change_entry("<virt-viewer>/view/toggle-
> > fullscreen", accel_key, accel_mods, TRUE);
> >          } else if (g_str_equal(*hotkey, "release-cursor")) {
> > diff --git a/tests/test-hotkeys.c b/tests/test-hotkeys.c
> > index d3658f5..4a45216 100644
> > --- a/tests/test-hotkeys.c
> > +++ b/tests/test-hotkeys.c
> > @@ -96,6 +96,10 @@ test_hotkeys_bad(void)
> >              "toggle-fullscreen=A,unknown_command=B",
> >              G_LOG_LEVEL_WARNING,
> >              "Unknown hotkey command unknown_command"
> > +        },{
> > +            "secure-attention=value",
> > +            G_LOG_LEVEL_WARNING,
> > +            "Invalid value 'value' for key 'secure-attention'"
> >          },
> >      };
> > 
> > --
> > 2.8.3
> > 
> > _______________________________________________
> > virt-tools-list mailing list
> > virt-tools-list at redhat.com
> > https://www.redhat.com/mailman/listinfo/virt-tools-list
> 
> ACK the code if it follows the suggestion (but feel free to convince
> me or here or on the iRC) :-)

I would rather send v2 :)

Pavel

> 
> Best Regards,
> --
> Fabiano Fidêncio