From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heinrich Schuchardt Date: Sat, 15 May 2021 19:16:51 +0200 Subject: [BUG] [PATCH v4 7/9] sandbox: Avoid using malloc() for system state In-Reply-To: <20210206095728.v4.7.I8f45b8640fe93b61c6e24cac4d70c193bccaf192@changeid> References: <20210206165736.3491584-1-sjg@chromium.org> <20210206095728.v4.7.I8f45b8640fe93b61c6e24cac4d70c193bccaf192@changeid> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 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); > >