[SLOF] [PATCH 3/5] Improve stack usage in libnvram
Alexey Kardashevskiy
aik at ozlabs.ru
Mon Nov 30 17:50:14 AEDT 2015
On 11/26/2015 06:58 AM, Thomas Huth wrote:
> The Forth-to-C wrapper code in libnvram.code uses a temporary
> buffer of 255 bytes via the STRING_INIT macro for each Forth string
> that has to be converted into a C string. However, using such big
> arrays in the wrapper code is a bad idea: Each of the buffers
> is put into the stack frame of the engine() function (from paflof.c)!
> That means the 7 strings from libnvram.code increase the stack
> usage of engine() by 7 * 255 = 1785 bytes! This can cause stack
> overflows since engine() can be called recursively, e.g. via the
> forth_eval() macro.
> To fix this issue, introduce proper wrapper functions for the libnvram
> code that has to deal with Forth strings, and handle the temporary
> buffers there instead. This way the stack frame of engine() is not
> overloaded by the temporary buffers anymore.
>
> Signed-off-by: Thomas Huth <thuth at redhat.com>
> ---
> lib/libnvram/envvar.c | 43 ++++++++++++++++++++++++++++++++++
> lib/libnvram/libnvram.code | 58 +++++++++++++++++++++-------------------------
> lib/libnvram/nvram.c | 10 ++++++++
> lib/libnvram/nvram.h | 5 ++++
> 4 files changed, 84 insertions(+), 32 deletions(-)
>
> diff --git a/lib/libnvram/envvar.c b/lib/libnvram/envvar.c
> index 87aaf27..1ac0a28 100644
> --- a/lib/libnvram/envvar.c
> +++ b/lib/libnvram/envvar.c
> @@ -80,6 +80,16 @@ char *get_env(partition_t part, char *envvar)
> return NULL;
> }
>
> +char *get_env_fs(partition_t part, char *envvar, int evlen)
> +{
> + char buf[evlen + 1];
> +
> + memcpy(buf, envvar, evlen);
> + buf[evlen] = 0;
> +
> + return get_env(part, buf);
> +}
> +
> static int find_last_envvar(partition_t part)
> {
> uint8_t last, current;
> @@ -132,6 +142,18 @@ int add_env(partition_t part, char *envvar, char *value)
> return 0;
> }
>
> +int add_env_fs(partition_t part, char *envvar, int evlen, char *val, int vallen)
> +{
> + char evbuf[evlen + 1], valbuf[vallen + 1];
> +
> + memcpy(evbuf, envvar, evlen);
> + evbuf[evlen] = 0;
> + memcpy(valbuf, val, vallen);
> + valbuf[vallen] = 0;
> +
> + return add_env(part, evbuf, valbuf);
> +}
> +
> int del_env(partition_t part, char *envvar)
> {
> int last, current, pos, i;
> @@ -168,6 +190,16 @@ int del_env(partition_t part, char *envvar)
> return 0;
> }
>
> +int del_env_fs(partition_t part, char *envvar, int evlen)
> +{
> + char buf[evlen + 1];
> +
> + memcpy(buf, envvar, evlen);
> + buf[evlen] = 0;
> +
> + return del_env(part, buf);
> +}
> +
> int set_env(partition_t part, char *envvar, char *value)
> {
> char *oldvalue, *buffer;
> @@ -241,3 +273,14 @@ int set_env(partition_t part, char *envvar, char *value)
> return 0; /* success */
> }
>
> +int set_env_fs(partition_t part, char *envvar, int evlen, char *val, int vallen)
> +{
> + char evbuf[evlen + 1], valbuf[vallen + 1];
> +
> + memcpy(evbuf, envvar, evlen);
> + evbuf[evlen] = 0;
> + memcpy(valbuf, val, vallen);
> + valbuf[vallen] = 0;
> +
> + return set_env(part, evbuf, valbuf);
> +}
> diff --git a/lib/libnvram/libnvram.code b/lib/libnvram/libnvram.code
> index 723941d..6d6d06a 100644
> --- a/lib/libnvram/libnvram.code
> +++ b/lib/libnvram/libnvram.code
> @@ -11,17 +11,6 @@
> *****************************************************************************/
> #include <nvram.h>
>
> -#define STRING_INIT(str) \
> - char str[255]; \
> - char * str##_address; \
> - int str##_length;
> -
> -#define STRING_FROM_STACK(str) \
> - str##_length = TOS.u; POP; \
> - str##_address = TOS.a; POP; \
> - memcpy(str, str##_address, str##_length); \
> - memset(str + str##_length, 0, 255 - str##_length);
> -
> PRIM(nvram_X2d_c_X40)
> unsigned int offset = TOS.u;
> TOS.u=nvram_read_byte(offset);
> @@ -80,21 +69,18 @@ MIRP
>
> /* get-named-nvram-partition ( name.addr name.len -- addr len FAILED? ) */
> PRIM(get_X2d_named_X2d_nvram_X2d_partition)
> - STRING_INIT(name)
> partition_t partition;
> + int namelen = TOS.n; POP;
>
> - STRING_FROM_STACK(name)
> - partition = get_partition(-1, name);
> + partition = get_partition_fs(-1, TOS.a, namelen);
>
> if(partition.len && partition.len != -1) {
> - PUSH;
> TOS.u = partition.addr;
> PUSH;
> TOS.u = partition.len;
> PUSH;
> TOS.u = 0; // FALSE
> } else {
> - PUSH;
> TOS.u = -1; // TRUE
> }
> MIRP
> @@ -165,15 +151,17 @@ MIRP
>
> // ( part.start part.len name.addr name.len -- var.addr var.len TRUE | false )
> PRIM(internal_X2d_get_X2d_env)
> - STRING_INIT(name)
> + char *name;
> + int namelen;
> partition_t part;
> char *val;
>
> - STRING_FROM_STACK(name)
> + namelen = TOS.u; POP;
> + name = TOS.a; POP;
> part.len = TOS.u; POP;
> part.addr = TOS.u; POP;
>
> - val=get_env(part, name);
> + val = get_env_fs(part, name, namelen);
> if(val) {
> PUSH; TOS.a = val;
> PUSH; TOS.u = strlen(val);
> @@ -185,17 +173,19 @@ MIRP
>
> // ( part.start part.len name.addr name.len val.addr val.len -- FALSE|TRUE)
> PRIM(internal_X2d_add_X2d_env)
> - STRING_INIT(name)
> - STRING_INIT(value)
> + char *name, *val;
> + int namelen, vallen;
> partition_t part;
> int ret;
>
> - STRING_FROM_STACK(value)
> - STRING_FROM_STACK(name)
> + vallen = TOS.u; POP;
> + val = TOS.a; POP;
> + namelen = TOS.u; POP;
> + name = TOS.a; POP;
> part.len = TOS.u; POP;
> part.addr = TOS.u; POP;
>
> - ret=add_env(part, name, value);
> + ret = add_env_fs(part, name, namelen, val, vallen);
> if(ret) {
> PUSH; TOS.u = -1; // TRUE
> } else {
> @@ -205,15 +195,17 @@ MIRP
>
> // ( part.addr part.len name.addr name.len -- FALSE|TRUE)
> PRIM(internal_X2d_del_X2d_env)
> - STRING_INIT(name)
> + char *name;
> + int namelen;
> partition_t part;
> int ret;
>
> - STRING_FROM_STACK(name);
> + namelen = TOS.u; POP;
> + name = TOS.a; POP;
> part.len = TOS.u; POP;
> part.addr = TOS.u; POP;
>
> - ret=del_env(part, name);
> + ret = del_env_fs(part, name, namelen);
> if(ret) {
> PUSH; TOS.u = -1; // TRUE
> } else {
> @@ -224,17 +216,19 @@ MIRP
>
> // internal-set-env ( part.addr part.len name.addr name.len val.addr val.len -- FALSE|TRUE)
> PRIM(internal_X2d_set_X2d_env)
> - STRING_INIT(name)
> - STRING_INIT(value)
> + char *name, *value;
> + int namelen, valuelen;
> partition_t part;
> int ret;
>
> - STRING_FROM_STACK(value)
> - STRING_FROM_STACK(name)
> + valuelen = TOS.u; POP;
> + value = TOS.a; POP;
> + namelen = TOS.u; POP;
> + name = TOS.a; POP;
> part.len = TOS.u; POP;
> part.addr = TOS.u; POP;
>
> - ret=set_env(part, name, value);
> + ret = set_env_fs(part, name, namelen, value, valuelen);
> if(ret) {
> PUSH; TOS.u = -1; // TRUE
> } else {
> diff --git a/lib/libnvram/nvram.c b/lib/libnvram/nvram.c
> index 5c11376..12251e3 100644
> --- a/lib/libnvram/nvram.c
> +++ b/lib/libnvram/nvram.c
> @@ -358,6 +358,16 @@ partition_t get_partition(unsigned int type, char *name)
> return ret;
> }
>
> +partition_t get_partition_fs(unsigned int type, char *name, int namelen)
> +{
> + char buf[namelen + 1];
> +
> + memcpy(buf, name, namelen);
> + buf[namelen] = 0;
> +
> + return get_partition(type, buf);
> +}
> +
> void erase_nvram(int offset, int len)
> {
> int i;
> diff --git a/lib/libnvram/nvram.h b/lib/libnvram/nvram.h
> index fa6bdd4..a1f3e67 100644
> --- a/lib/libnvram/nvram.h
> +++ b/lib/libnvram/nvram.h
> @@ -51,6 +51,7 @@ char *get_nvram_buffer(int len);
> void free_nvram_buffer(char *buffer);
> int nvramlog_printf(const char* fmt, ...);
> partition_t get_partition(unsigned int type, char *name);
> +partition_t get_partition_fs(unsigned int type, char *name, int namelen);
> void erase_nvram(int offset, int len);
> int wipe_partition(partition_t partition, int header_only);
> partition_t new_nvram_partition(int type, char *name, int len);
> @@ -66,8 +67,12 @@ unsigned int get_nvram_size(void);
>
> /* envvar.c */
> char *get_env(partition_t part, char *envvar);
> +char *get_env_fs(partition_t part, char *envvar, int evnamelen);
> int add_env(partition_t part, char *envvar, char *value);
> +int add_env_fs(partition_t part, char *envvar, int evlen, char *val, int vlen);
> int del_env(partition_t part, char *envvar);
> +int del_env_fs(partition_t part, char *envvar, int evlen);
> int set_env(partition_t part, char *envvar, char *value);
> +int set_env_fs(partition_t part, char *envvar, int evlen, char *val, int vlen);
The patch is pretty good as is but it also looks like that instead of
introducing xxx_fs() helpers, you could simply add the length parameters to
the existing get_env/add_env/del_env/set_env as pretty much all code down
there uses strncmp() already and calls strlen() on every passed string.
--
Alexey
More information about the SLOF
mailing list