[BUG] [PATCH v4 7/9] sandbox: Avoid using malloc() for system state

Heinrich Schuchardt xypron.glpk at gmx.de
Sat May 15 19:16:51 CEST 2021


On 2/6/21 5:57 PM, Simon Glass wrote:
> This state is not accessible to the running U-Boot but at present it is
> allocated in the emulated SDRAM. This doesn't seem very useful. Adjust
> it to allocate from the OS instead.
>
> The RAM buffer is currently not freed, but should be, so add that into
> state_uninit(). Update the comment for os_free() to indicate that NULL is
> a valid parameter value.
>
> Note that the strdup() in spl_board_load_image() is changed as well, since
> strdup() allocates memory in the RAM buffer.
>
> Signed-off-by: Simon Glass <sjg at chromium.org>

This patch was merged as commit b308d9fd18fa4b9b.

It breaks the exception handling:

Before the patch if an exception occured pc_reloc would point to the
address in u-boot.map:

=> exception undef
Illegal instruction
pc = 0x55e4ef74c434, pc_reloc = 0x55434

With your patch pc_reloc is incorrect:

=> exception undef
Illegal instruction
pc = 0x56003896cd13, pc_reloc = 0x56002896cd13

Why is gd->reloc_off not filled correctly anymore?

os_find_text_base() has this comment:

"This code assumes that the first line of /proc/self/maps holds
information about the text."

$ cat /proc/389589/maps
10000000-10002000 rwxp 00000000 00:00 0
560038916000-560038947000 r--p 00000000 fd:01 13376716
560038947000-560038aa4000 r-xp 00031000 fd:01 13376716
560038aa4000-560038ba8000 r--p 0018e000 fd:01 13376716
560038ba8000-560038bb1000 r--p 00291000 fd:01 13376716
560038bb1000-560038bd4000 rw-p 0029a000 fd:01 13376716

=> bdinfo
relocaddr   = 0x0000000007858000
reloc off   = 0x0000000010000000

The problem is that you first call mmap() to allocate space for the
state at address 0x10000000 and then call os_find_text_base(). Calling
os_find_text_base() must be the first thing you do!

Best regards

Heinrich


> ---
>
> (no changes since v2)
>
> Changes in v2:
> - Drop unnecessary check before calling os_free()
> - Update the comment for os_free()
> - Also free the RAM buffer
> - Convert the rest of the allocations in os.c, etc.
>
>   arch/sandbox/cpu/os.c    | 24 ++++++++++++------------
>   arch/sandbox/cpu/spl.c   | 10 +++++++---
>   arch/sandbox/cpu/start.c | 12 +++++++-----
>   arch/sandbox/cpu/state.c | 18 +++++++++---------
>   include/os.h             |  3 ++-
>   5 files changed, 37 insertions(+), 30 deletions(-)
>
> diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
> index d23aad318be..f5000e64966 100644
> --- a/arch/sandbox/cpu/os.c
> +++ b/arch/sandbox/cpu/os.c
> @@ -153,7 +153,7 @@ int os_read_file(const char *fname, void **bufp, int *sizep)
>   		printf("Cannot seek to start of file '%s'\n", fname);
>   		goto err;
>   	}
> -	*bufp = malloc(size);
> +	*bufp = os_malloc(size);
>   	if (!*bufp) {
>   		printf("Not enough memory to read file '%s'\n", fname);
>   		ret = -ENOMEM;
> @@ -391,8 +391,8 @@ int os_parse_args(struct sandbox_state *state, int argc, char *argv[])
>   	state->argv = argv;
>
>   	/* dynamically construct the arguments to the system getopt_long */
> -	short_opts = malloc(sizeof(*short_opts) * num_options * 2 + 1);
> -	long_opts = malloc(sizeof(*long_opts) * (num_options + 1));
> +	short_opts = os_malloc(sizeof(*short_opts) * num_options * 2 + 1);
> +	long_opts = os_malloc(sizeof(*long_opts) * (num_options + 1));
>   	if (!short_opts || !long_opts)
>   		return 1;
>
> @@ -471,7 +471,7 @@ void os_dirent_free(struct os_dirent_node *node)
>
>   	while (node) {
>   		next = node->next;
> -		free(node);
> +		os_free(node);
>   		node = next;
>   	}
>   }
> @@ -496,7 +496,7 @@ int os_dirent_ls(const char *dirname, struct os_dirent_node **headp)
>   	/* Create a buffer upfront, with typically sufficient size */
>   	dirlen = strlen(dirname) + 2;
>   	len = dirlen + 256;
> -	fname = malloc(len);
> +	fname = os_malloc(len);
>   	if (!fname) {
>   		ret = -ENOMEM;
>   		goto done;
> @@ -509,7 +509,7 @@ int os_dirent_ls(const char *dirname, struct os_dirent_node **headp)
>   			ret = errno;
>   			break;
>   		}
> -		next = malloc(sizeof(*node) + strlen(entry->d_name) + 1);
> +		next = os_malloc(sizeof(*node) + strlen(entry->d_name) + 1);
>   		if (!next) {
>   			os_dirent_free(head);
>   			ret = -ENOMEM;
> @@ -518,10 +518,10 @@ int os_dirent_ls(const char *dirname, struct os_dirent_node **headp)
>   		if (dirlen + strlen(entry->d_name) > len) {
>   			len = dirlen + strlen(entry->d_name);
>   			old_fname = fname;
> -			fname = realloc(fname, len);
> +			fname = os_realloc(fname, len);
>   			if (!fname) {
> -				free(old_fname);
> -				free(next);
> +				os_free(old_fname);
> +				os_free(next);
>   				os_dirent_free(head);
>   				ret = -ENOMEM;
>   				goto done;
> @@ -555,7 +555,7 @@ int os_dirent_ls(const char *dirname, struct os_dirent_node **headp)
>
>   done:
>   	closedir(dir);
> -	free(fname);
> +	os_free(fname);
>   	return ret;
>   }
>
> @@ -672,7 +672,7 @@ static int add_args(char ***argvp, char *add_args[], int count)
>   	for (argc = 0; (*argvp)[argc]; argc++)
>   		;
>
> -	argv = malloc((argc + count + 1) * sizeof(char *));
> +	argv = os_malloc((argc + count + 1) * sizeof(char *));
>   	if (!argv) {
>   		printf("Out of memory for %d argv\n", count);
>   		return -ENOMEM;
> @@ -755,7 +755,7 @@ static int os_jump_to_file(const char *fname)
>   		os_exit(2);
>
>   	err = execv(fname, argv);
> -	free(argv);
> +	os_free(argv);
>   	if (err) {
>   		perror("Unable to run image");
>   		printf("Image filename '%s'\n", fname);
> diff --git a/arch/sandbox/cpu/spl.c b/arch/sandbox/cpu/spl.c
> index 9a77da15619..197b98c9828 100644
> --- a/arch/sandbox/cpu/spl.c
> +++ b/arch/sandbox/cpu/spl.c
> @@ -42,10 +42,14 @@ static int spl_board_load_image(struct spl_image_info *spl_image,
>   		return ret;
>   	}
>
> -	/* Set up spl_image to boot from jump_to_image_no_args() */
> -	spl_image->arg = strdup(fname);
> +	/*
> +	 * Set up spl_image to boot from jump_to_image_no_args(). Allocate this
> +	 * outsdide the RAM buffer (i.e. don't use strdup()).
> +	 */
> +	spl_image->arg = os_malloc(strlen(fname) + 1);
>   	if (!spl_image->arg)
> -		return log_msg_ret("Setup exec filename", -ENOMEM);
> +		return log_msg_ret("exec", -ENOMEM);
> +	strcpy(spl_image->arg, fname);
>
>   	return 0;
>   }
> diff --git a/arch/sandbox/cpu/start.c b/arch/sandbox/cpu/start.c
> index 25425809747..295f36c0bda 100644
> --- a/arch/sandbox/cpu/start.c
> +++ b/arch/sandbox/cpu/start.c
> @@ -87,7 +87,7 @@ int sandbox_early_getopt_check(void)
>
>   	/* Sort the options */
>   	size = sizeof(*sorted_opt) * num_options;
> -	sorted_opt = malloc(size);
> +	sorted_opt = os_malloc(size);
>   	if (!sorted_opt) {
>   		printf("No memory to sort options\n");
>   		os_exit(1);
> @@ -187,7 +187,7 @@ static int sandbox_cmdline_cb_default_fdt(struct sandbox_state *state,
>   	int len;
>
>   	len = strlen(state->argv[0]) + strlen(fmt) + 1;
> -	fname = malloc(len);
> +	fname = os_malloc(len);
>   	if (!fname)
>   		return -ENOMEM;
>   	snprintf(fname, len, fmt, state->argv[0]);
> @@ -207,7 +207,7 @@ static int sandbox_cmdline_cb_test_fdt(struct sandbox_state *state,
>   	int len;
>
>   	len = strlen(state->argv[0]) + strlen(fmt) + 1;
> -	fname = malloc(len);
> +	fname = os_malloc(len);
>   	if (!fname)
>   		return -ENOMEM;
>   	strcpy(fname, state->argv[0]);
> @@ -435,16 +435,18 @@ int main(int argc, char *argv[])
>   {
>   	struct sandbox_state *state;
>   	gd_t data;
> +	int size;
>   	int ret;
>
>   	/*
>   	 * Copy argv[] so that we can pass the arguments in the original
>   	 * sequence when resetting the sandbox.
>   	 */
> -	os_argv = calloc(argc + 1, sizeof(char *));
> +	size = sizeof(char *) * (argc + 1);
> +	os_argv = os_malloc(size);
>   	if (!os_argv)
>   		os_exit(1);
> -	memcpy(os_argv, argv, sizeof(char *) * (argc + 1));
> +	memcpy(os_argv, argv, size);
>
>   	memset(&data, '\0', sizeof(data));
>   	gd = &data;
> diff --git a/arch/sandbox/cpu/state.c b/arch/sandbox/cpu/state.c
> index b2901b7a8ca..4ffaf163789 100644
> --- a/arch/sandbox/cpu/state.c
> +++ b/arch/sandbox/cpu/state.c
> @@ -29,17 +29,17 @@ static int state_ensure_space(int extra_size)
>   		return 0;
>
>   	size = used + extra_size;
> -	buf = malloc(size);
> +	buf = os_malloc(size);
>   	if (!buf)
>   		return -ENOMEM;
>
>   	ret = fdt_open_into(blob, buf, size);
>   	if (ret) {
> -		free(buf);
> +		os_free(buf);
>   		return -EIO;
>   	}
>
> -	free(blob);
> +	os_free(blob);
>   	state->state_fdt = buf;
>   	return 0;
>   }
> @@ -55,7 +55,7 @@ static int state_read_file(struct sandbox_state *state, const char *fname)
>   		printf("Cannot find sandbox state file '%s'\n", fname);
>   		return -ENOENT;
>   	}
> -	state->state_fdt = malloc(size);
> +	state->state_fdt = os_malloc(size);
>   	if (!state->state_fdt) {
>   		puts("No memory to read sandbox state\n");
>   		return -ENOMEM;
> @@ -77,7 +77,7 @@ static int state_read_file(struct sandbox_state *state, const char *fname)
>   err_read:
>   	os_close(fd);
>   err_open:
> -	free(state->state_fdt);
> +	os_free(state->state_fdt);
>   	state->state_fdt = NULL;
>
>   	return ret;
> @@ -244,7 +244,7 @@ int sandbox_write_state(struct sandbox_state *state, const char *fname)
>   	/* Create a state FDT if we don't have one */
>   	if (!state->state_fdt) {
>   		size = 0x4000;
> -		state->state_fdt = malloc(size);
> +		state->state_fdt = os_malloc(size);
>   		if (!state->state_fdt) {
>   			puts("No memory to create FDT\n");
>   			return -ENOMEM;
> @@ -302,7 +302,7 @@ int sandbox_write_state(struct sandbox_state *state, const char *fname)
>   err_write:
>   	os_close(fd);
>   err_create:
> -	free(state->state_fdt);
> +	os_free(state->state_fdt);
>
>   	return ret;
>   }
> @@ -419,8 +419,8 @@ int state_uninit(void)
>   	if (state->jumped_fname)
>   		os_unlink(state->jumped_fname);
>
> -	if (state->state_fdt)
> -		free(state->state_fdt);
> +	os_free(state->state_fdt);
> +	os_free(state->ram_buf);
>   	memset(state, '\0', sizeof(*state));
>
>   	return 0;
> diff --git a/include/os.h b/include/os.h
> index fd010cfee83..d2a4afeca0f 100644
> --- a/include/os.h
> +++ b/include/os.h
> @@ -123,7 +123,8 @@ void *os_malloc(size_t length);
>    *
>    * This returns the memory to the OS.
>    *
> - * @ptr:	Pointer to memory block to free
> + * @ptr:	Pointer to memory block to free. If this is NULL then this
> + *		function does nothing
>    */
>   void os_free(void *ptr);
>
>



More information about the U-Boot mailing list