[SLOF] [PATCH 3/5] Improve stack usage in libnvram
Thomas Huth
thuth at redhat.com
Tue Dec 1 05:57:52 AEDT 2015
On 30/11/15 07:50, Alexey Kardashevskiy wrote:
> 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.
True ... my original idea was to keep the functions since this is a
library and also might be called from other C code ... but apparently,
the functions are currently only called from the Forth wrapper so far.
So I'll have a try and rework my patch, let's see how it looks like then...
Thomas
More information about the SLOF
mailing list