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

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@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);