[ABRT PATCH] add environment variable whitelist to debuginfo install wrapper - closes 692

Jakub Filak jfilak at redhat.com
Tue Sep 10 13:43:38 UTC 2013


It looks good to me except some coding style issues.


> +        // However since we communicate through environment variables
> +        // we have to keep a whitelist of variables to keep.
> +        static const char *whitelist[] = {
> +            "REPORT_CLIENT_SLAVE" //  Check if the app is being run as a slave
> +        };
> +        int wlsize = sizeof(whitelist)/sizeof(char*);

I'd use a more accurate type. Maybe size_t or rather const size_t.

> +        char *setlist[wlsize];

You can statically initialize the entire array to NULLs by assigning
NULL to the first element.

           char *setlist[sizeof(whitelist)/sizeof(char*)] = { NULL };

> +        char *p = NULL;
> +        for (int i = 0; i < wlsize; i++) {

           for (size_t i = 0; i < wlsize; ++i)
           {

> +            if ((p = getenv(whitelist[i])) != NULL) 
               {
> +                setlist[i] = malloc(strlen(p)*sizeof(char*));
> +                setlist[i] = strncpy(setlist[i], p, strlen(p));

How about:
                   setlist[i] = xstrdup(p);			
> +            }
> +            else
> +                setlist[i] = NULL;

And if the array is initialized to NULLs, you can drop the two above
lines.

> +        }
> +        // Now we can clear the environment
>          clearenv();
> +        // And once again set whitelisted variables
> +        for (int i = 0; i < wlsize; i++)
           {
> +            if (setlist[i] != NULL)
               {

How about: 
                   xsetenv(whitelist[i], setlist[i]);
> +                free(setlist[i]);
> +            }
> +        }
>  #else
>          /* Clear dangerous stuff from env */
>          static const char forbid[] =




More information about the Crash-catcher mailing list