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

[Mark Hatle <mark.hatle@windriver.com>]

On 8/25/13 7:40 PM, Peter A. Bigot wrote:
> From: "Peter A. Bigot" <pab@pabigot.com>
>
> qemu.bbclass adds PSEUDO_UNLOAD=1 in qemu_run_binary to avoid reference to
> pseudo functions that may not exist in the target environment.  This patch
> detects the addition of that variable within the environment to which the
> call applies, even if not present in the parent environment.
>
> As a side effect it fixes a memory leak.

Pseudo maintainer is on vacation this week.. so I'm not sure if he'll respond or 
not.  But I'll take a whack at it.

> [YOCTO #4843]
>
> Signed-off-by: Peter A. Bigot <pab@pabigot.com>
> ---
>   .../0001-pseudo_has_unload-add-function.patch      |  190 ++++++++++++++++++++
>   meta/recipes-devtools/pseudo/pseudo_1.5.1.bb       |    7 +-
>   2 files changed, 195 insertions(+), 2 deletions(-)
>   create mode 100644 meta/recipes-devtools/pseudo/files/0001-pseudo_has_unload-add-function.patch
>
> diff --git a/meta/recipes-devtools/pseudo/files/0001-pseudo_has_unload-add-function.patch b/meta/recipes-devtools/pseudo/files/0001-pseudo_has_unload-add-function.patch
> new file mode 100644
> index 0000000..b5c81c9
> --- /dev/null
> +++ b/meta/recipes-devtools/pseudo/files/0001-pseudo_has_unload-add-function.patch
> @@ -0,0 +1,190 @@
> +From be97cb958f2934fa398fc8e344b25b84ebd4e90c Mon Sep 17 00:00:00 2001
> +From: "Peter A. Bigot" <pab@pabigot.com>
> +Date: Sun, 25 Aug 2013 19:22:09 -0500
> +Subject: [PATCH] pseudo_has_unload: add function
> +
> +Various wrappers checked for a non-null pseudo_get_value("PSEUDO_UNLOAD") to
> +determine whether the environment should include the pseudo variables.  None
> +of those checks freed the returned value when it was not null.  The new
> +check function does.
> +
> +The new check function also sees whether PSEUDO_UNLOAD was defined in the
> +environment that should be used in the wrapped system call.  This allows
> +pkg_postinst scripts to strip out the LD_PRELOAD setting, for example before
> +invoking qemu to execute commands in an environment that does not have
> +libpseudo.so.
> +
> +[YOCTO #4843]
> +
> +Upstream-Status: Pending
> +Signed-off-by: Peter A. Bigot <pab@pabigot.com>
> +---
> + ports/common/guts/execv.c              |    2 +-
> + ports/common/guts/execve.c             |    2 +-
> + ports/common/guts/execvp.c             |    2 +-
> + ports/common/guts/fork.c               |    2 +-
> + ports/linux/newclone/pseudo_wrappers.c |    2 +-
> + ports/linux/oldclone/pseudo_wrappers.c |    2 +-
> + ports/unix/guts/popen.c                |    2 +-
> + ports/unix/guts/system.c               |    2 +-
> + pseudo.h                               |    1 +
> + pseudo_util.c                          |   27 +++++++++++++++++++++++++++
> + 10 files changed, 36 insertions(+), 8 deletions(-)
> +
> +diff --git a/ports/common/guts/execv.c b/ports/common/guts/execv.c
> +index 763e1f9..3e1f820 100644
> +--- a/ports/common/guts/execv.c
> ++++ b/ports/common/guts/execv.c
> +@@ -19,7 +19,7 @@
> + 	}
> +
> + 	pseudo_setupenv();
> +-	if (pseudo_get_value("PSEUDO_UNLOAD"))
> ++	if (pseudo_has_unload(NULL))

Ya, there is definitely a memory leak here.. the pseudo_get_value says:

/* Caller must free memory! */
char *
pseudo_get_value(const char *key) {


....

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

--Mark