[VDPAU] [PATCH] Add detection for multiple flash substrings

[Emil Velikov]

Hello Sergey,

On 20 May 2015 at 07:25, Sergey Frolov <dunkan.aidaho at gmail.com> wrote:
> Hi José,
>
> Thanks for your reply.
>
>> I think this is a nice change. I never liked having
>> "libflashplayer.so" hard coded in vdpau_wrapper.c.
>>
>> I worry, though, that this change exposes us to churn based on
>> Mozilla and Opera policy decision about what they do with their
>> handling of the Flash plugin. However, my observation has been that
>> these policies do not change often. In fact the original motivation
>> for having Flash detection was Adobe's unwillingness to fix their
>> bugs.
>
> My thinking behind providing default settings to vdpau_wrapper.cfg was:
> it was a default behaviour for some time now, let someone else break it
> (Adobe). And it's well advertised too, since when I was finally able to
> experience the issue years after everyone else (thanks to
> libvdpau-va-gl1 on Intel HD3000), I knew where to look for solution.
>
> That's when I was greeted with hardcoded value.
>
>> How is _flash_process_signatures initialized or terminated? This line
>> worries me:
>>
>>  for (signature = _flash_process_signatures[0]; signature != NULL;
>>       signature++) {
>>
>> If static char _flash_process_signatures[16][64] is uninitialized and
>> contains random data this loop will run off into the weeds. We also
>> need to assume that a user will modify their vdpau_wrapper.cfg and
>> not have any workarounds (unlikely, but this must work correctly). Or
>> maybe I am wrong here; if so please tell me how.
The C99 standard dictates (6.7.8.10 Initialization) that the variable
will be implicitly set to zero/null. Personally I feel that it's
better to keep things shorter (cleaner) and leave it as is, although
others may feel differently [1].

>
> Yeah, I've totally missed this case, now I'm initializing array of
> pointers with NULL.
>
>> Using a statically sized array means VDPAU will only support the
>> first N entries of flash_process=. This is okay but should be
>> documented.
>
> I understand why statically allocated chunk of memory may hurt user
> experience along with developer's common sense. This was a quick
> decision in order to make it work, made in "ought to be enough for
> everybody" mentality. Hypothetically, somewhere for someone that might
> not be true.
>
> I propose a middle ground here: statically allocated array of pointers
> enough for about 8-16 times amount of flash applications on *nix
> systems with no explicit restrictions on length of the parameter.
>
>>
>> Mechanical nit: I prefer "char *signature" instead of "char *
>> signature". If you do not do this I will do so in a follow on change.
>
> Corrected.
>
>
> Since I've finished K&R a week before I sent this patch any of your
> remarks are welcome.
>
> Regards,
> Sergey.
>
>
> diff --git a/src/vdpau_wrapper.c b/src/vdpau_wrapper.c
> index 8efbd39..5d3a347 100644
> --- a/src/vdpau_wrapper.c
> +++ b/src/vdpau_wrapper.c
> @@ -239,6 +239,7 @@ static void _vdp_close_driver(void)
>  static VdpGetProcAddress * _imp_get_proc_address;
>  static VdpVideoSurfacePutBitsYCbCr * _imp_vid_put_bits_y_cb_cr;
>  static VdpPresentationQueueSetBackgroundColor * _imp_pq_set_bg_color;
> +static char *_flash_process_signatures[128] = {NULL};
As mentioned above - initialising to null can be omitted.

> @@ -346,8 +348,11 @@ static void init_running_under_flash(void)
>      }
>      buffer[ret] = '\0';
>
> -    if (strstr(buffer, "libflashplayer") != NULL) {
> -        _running_under_flash = 1;
> +    for (signature = &_flash_process_signatures[0]; *signature !=
> NULL; signature++) {
Please wrap this long line.

> @@ -378,6 +387,11 @@ static void init_config(void)
>          else if (!strcmp(buffer, "disable_flash_pq_bg_color")) {
>              _disable_flash_pq_bg_color = atoi(param);
>          }
> +        else if (!strcmp(buffer, "flash_process")) {
An ill minded person can easily wreck havoc considering the fixed size
of _flash_process_signatures. Perhaps ignoring (and printing a warning
message) if more than 128 flash_process entries are processed, would
be preferred ?

> +            *signature = (char *) malloc(strlen(param)+1);
> +            strncpy(*signature, param, strlen(param));
The last (terminating null) byte seems to be uninitialised. Also we'll
likely crash if malloc fails.
Using strdup (and not incrementing signature if it fails) looks like a
shorter solution imho.


Hope you find this useful.

Cheers
Emil

[1] http://stackoverflow.com/questions/1294772/does-gcc-automatically-initialize-static-variables-to-zero