[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