Re: [PATCH] pseudo: fix memory leak and missed privilege drop

["Peter A. Bigot" <pab@pabigot.com>]

On 08/26/2013 10:54 AM, Mark Hatle wrote:
> +diff --git a/pseudo_util.c b/pseudo_util.c
>> +index 8d0969e..16c70e0 100644
>> +--- a/pseudo_util.c
>> ++++ b/pseudo_util.c
>> +@@ -95,6 +95,33 @@ dump_env(char **envp) {
>> + }
>> + #endif
>> +
>> ++int
>> ++pseudo_has_unload(char * const *envp) {
>> ++    static const char unload[] = "PSEUDO_UNLOAD";
>> ++    static size_t unload_len = strlen(unload);
>> ++    size_t i = 0;
>> ++
>> ++    /* Is it in the caller environment? */
>> ++    if (NULL != getenv(unload))
>> ++        return 1;
>
> We actually want to use the pseudo_get_value as that ensures that the 
> internal environment configuration is sane at all times.  The issue is 
> that applications can do odd things to the environment, and just 
> checking the environment itself may not always give sane results.
>
> The rule for variables is:
>
> * PSEUDO Variables are loaded ASAP into a cache.  This way if the user 
> or program clear or modify the environment we maintain the variable.
>
> * To 'reset' the variables to different values an exec needs to be 
> performed. (Prior to the exec we also restore any missing variables 
> that have been cached to the environment.)
>
> -however- PSEUDO_UNLOAD is supposed to be special and acted on at the 
> vary next exec...  So something like the following:
>
> int
> pseudo_has_unload(char * const *envp) {
>     static const char unload[] = "PSEUDO_UNLOAD";
>     static size_t unload_len = strlen(unload);
>     char * value = NULL;
>     size_t i = 0;
>
>     value = pseudo_get_value(unload)
>     if (value) {
>         free(value);
>         return 1;
>     }
>     free(value);
>
>     /* Is it in the caller environment? */
>     if (NULL != getenv(unload))
>         return 1;
>
>     /* Is it in the operational environment? */
>     while (envp && *envp) {
>         if ((!strncmp(*envp, unload, unload_len)) && ('=' == 
> (*envp)[unload_len]))
>             return 1;
>         ++envp;
>     }
>     return 0;
> }
>
> This is -almost- the same as what you had.  The difference is that we 
> check the cached value first, and use the get_value function which 
> does all of the setup and comparison using the common function.  This 
> ensures that the environment cache is properly configured as early as 
> possible, only one accessor of the cache is used and it also checks 
> the running and operation cache for the alternative value (in the same 
> way your patch did it).

Sure; I figured this was a special case, there was no point in 
allocating and immediately throwing away a value, and that since nobody 
looked at the value there was no need to update the cache through this 
call and the other cache routines would do it if necessary.  But the 
intent of the API wasn't entirely clear.  Based on what you said I'd 
probably refactor pseudo_get_value() so it and pseudo_has_unload() would 
share the code that located a matching cache entry, then duplicate the 
return value only to satisfy the API for pseudo_get_value().  Checking 
for PSEUDO_UNLOAD happens a lot in bitbaking images, so making it fast 
seems worthwhile.

I'm happy to leave this in your hands, including whether the fix 
includes a new version of pseudo or just a patch in the recipe.

Peter