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

[Fabiano Fidêncio]

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

Why? :-)

key++; would be way cleaner as we ened up using it here in your patch
and somewhere below in the same loop.

Okay, getting back to your patch ... strlen(key + 1) != 0 ... does it
really happen? what's the case? 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 :-\. Even though, I understand we should treat
this case ... so I'd go for: if (key == NULL || ++key == 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.

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

Best Regards,
--
Fabiano Fidêncio