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

[Fabiano Fidêncio]

On Tue, May 31, 2016 at 8:30 AM, Pavel Grunt <pgrunt at redhat.com> wrote:
> 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.

That's true, hotkey is still used and I was just paying attention to
the key. :-\
My bad. But I still would do: *key = '\0''; key++ instead of keeping
using key + 1.

>
>> 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.

Let me change my question then, does it really fail at some point?
Because, if it does, then we are in a situation where the hotkey is:
"foo=", which shouldn't never happend.

>
> 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.
>

Okay, so, please, add this in the comment.

>> 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.

Yep, as I said before, I didn't check that the hotkey would still be
used, my bad.
Just ignore my comment from the previous email.

>
>>  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'

Nops, it was just an way to avoid your strlen(key + 1) != 0 check, but
can just be ignored.

>
>>
>> > +            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 :)

Please!

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


Best Regards,