[LEDE-DEV] [PATCH libubox 1/3] Fix various memory management issues
Jo-Philipp Wich
jo at mein.io
Tue Jun 21 03:32:48 PDT 2016
Hi,
first of all, thanks for putting work into that - I like the changes in
general. Have a few comments inline below.
~ Jo
> Consistently handle allocation failures. Some functions are changed to
> return bool instead of void to allow returning an error.
>
> Also fix a buffer size miscalculation in lua/uloop.
>
> Signed-off-by: Matthias Schiffer <mschiffer at universe-factory.net>
> ---
> blobmsg.c | 20 +++++++++++++++-----
> blobmsg.h | 4 ++--
> blobmsg_json.c | 26 +++++++++++++++++++-------
> jshn.c | 4 ++++
> json_script.c | 17 +++++++++++++++--
> kvlist.c | 11 ++++++++---
> kvlist.h | 2 +-
> lua/uloop.c | 5 ++++-
> ustream.c | 7 +++++++
> utils.c | 2 ++
> 10 files changed, 77 insertions(+), 21 deletions(-)
>
> diff --git a/blobmsg.c b/blobmsg.c
> index 80b984a..1529fbc 100644
> --- a/blobmsg.c
> +++ b/blobmsg.c
> @@ -227,29 +227,38 @@ blobmsg_open_nested(struct blob_buf *buf, const char *name, bool array)
> return (void *)offset;
> }
>
> -void
> +bool
> blobmsg_vprintf(struct blob_buf *buf, const char *name, const char *format, va_list arg)
> {
> va_list arg2;
> char cbuf;
> + char *sbuf;
> int len;
>
> va_copy(arg2, arg);
> len = vsnprintf(&cbuf, sizeof(cbuf), format, arg2);
> va_end(arg2);
>
> - vsprintf(blobmsg_alloc_string_buffer(buf, name, len + 1), format, arg);
> + sbuf = blobmsg_alloc_string_buffer(buf, name, len + 1);
> + if (!sbuf)
> + return false;
> + vsprintf(sbuf, format, arg);
> blobmsg_add_string_buffer(buf);
> +
> + return true;
The vsprintf() we're wrapping here returns the number of bytes written,
which is useful imho. What about returning >= 0 bytes for success and -1
for error (alloc fail) ?
> }
>
> -void
> +bool
> blobmsg_printf(struct blob_buf *buf, const char *name, const char *format, ...)
> {
> va_list ap;
> + bool ret;
>
> va_start(ap, format);
> - blobmsg_vprintf(buf, name, format, ap);
> + ret = blobmsg_vprintf(buf, name, format, ap);
> va_end(ap);
> +
> + return ret;
Same, if we decide to return the bytes in blobmsg_vprintf() we need to
adjust the return type here.
> }
>
> void *
> @@ -278,7 +287,8 @@ blobmsg_realloc_string_buffer(struct blob_buf *buf, unsigned int maxlen)
> if (required <= 0)
> goto out;
>
> - blob_buf_grow(buf, required);
> + if (!blob_buf_grow(buf, required))
> + return NULL;
> attr = blob_next(buf->head);
>
> out:
> diff --git a/blobmsg.h b/blobmsg.h
> index e58f95d..7153565 100644
> --- a/blobmsg.h
> +++ b/blobmsg.h
> @@ -224,8 +224,8 @@ void *blobmsg_alloc_string_buffer(struct blob_buf *buf, const char *name, unsign
> void *blobmsg_realloc_string_buffer(struct blob_buf *buf, unsigned int maxlen);
> void blobmsg_add_string_buffer(struct blob_buf *buf);
>
> -void blobmsg_vprintf(struct blob_buf *buf, const char *name, const char *format, va_list arg);
> -void blobmsg_printf(struct blob_buf *buf, const char *name, const char *format, ...)
> +bool blobmsg_vprintf(struct blob_buf *buf, const char *name, const char *format, va_list arg);
> +bool blobmsg_printf(struct blob_buf *buf, const char *name, const char *format, ...)
> __attribute__((format(printf, 3, 4)));
>
>
> diff --git a/blobmsg_json.c b/blobmsg_json.c
> index 5713948..33f3969 100644
> --- a/blobmsg_json.c
> +++ b/blobmsg_json.c
> @@ -123,10 +123,13 @@ static bool blobmsg_puts(struct strbuf *s, const char *c, int len)
> return true;
>
> if (s->pos + len >= s->len) {
> - s->len += 16 + len;
> - s->buf = realloc(s->buf, s->len);
> - if (!s->buf)
> + size_t new_len = s->len + 16 + len;
> + char *new = realloc(s->buf, new_len);
Can we move the declarations to the top of the function and avoid "new"
as it is a C++ reserved word? You could call them "buf" and "buf_len" or
"cpy" and "cpy_len" maybe.
> + if (!new)
> return false;
> +
> + s->len = new_len;
> + s->buf = new;
> }
> memcpy(s->buf + s->pos, c, len);
> s->pos += len;
> @@ -290,14 +293,18 @@ char *blobmsg_format_json_with_cb(struct blob_attr *attr, bool list, blobmsg_jso
> {
> struct strbuf s;
> bool array;
> + char *ret;
>
> s.len = blob_len(attr);
> - s.buf = malloc(s.len);
> s.pos = 0;
> s.custom_format = cb;
> s.priv = priv;
> s.indent = false;
>
> + s.buf = malloc(s.len);
> + if (!s.buf)
> + return NULL;
> +
> if (indent >= 0) {
> s.indent = true;
> s.indent_level = indent;
> @@ -316,8 +323,13 @@ char *blobmsg_format_json_with_cb(struct blob_attr *attr, bool list, blobmsg_jso
> return NULL;
> }
>
> - s.buf = realloc(s.buf, s.pos + 1);
> - s.buf[s.pos] = 0;
> + ret = realloc(s.buf, s.pos + 1);
> + if (!ret) {
> + free(s.buf);
> + return NULL;
> + }
>
> - return s.buf;
> + ret[s.pos] = 0;
Did you forget to assign "ret" to "s.buf" here?
> +
> + return ret;
> }
> diff --git a/jshn.c b/jshn.c
> index e2d9022..4989099 100644
> --- a/jshn.c
> +++ b/jshn.c
> @@ -338,6 +338,10 @@ int main(int argc, char **argv)
> for (i = 0; environ[i]; i++);
>
> vars = calloc(i, sizeof(*vars));
> + if (!vars) {
> + fprintf(stderr, "%m\n");
The "%m" format is a glibc extension, I'd favor an explicit "%s" +
strerror(errno) here just to avoid potential portability hassle.
> + return -1;
> + }
> for (i = 0; environ[i]; i++) {
> char *c;
>
> diff --git a/json_script.c b/json_script.c
> index b5d414d..552ed61 100644
> --- a/json_script.c
> +++ b/json_script.c
> @@ -45,6 +45,9 @@ json_script_file_from_blobmsg(const char *name, void *data, int len)
> name_len = strlen(name) + 1;
>
> f = calloc_a(sizeof(*f) + len, &new_name, name_len);
> + if (!f)
> + return NULL;
> +
> memcpy(f->data, data, len);
> if (name)
> f->avl.key = strcpy(new_name, name);
> @@ -426,12 +429,15 @@ static int eval_string(struct json_call *call, struct blob_buf *buf, const char
> char c = '%';
>
> dest = blobmsg_alloc_string_buffer(buf, name, 1);
> + if (!dest)
> + return -1;
> +
> next = alloca(strlen(pattern) + 1);
> strcpy(next, pattern);
>
> for (str = next; str; str = next) {
> const char *cur;
> - char *end;
> + char *end, *new;
Can we avoid "new" here and use "cpy" or "copy" instead?
> int cur_len = 0;
> bool cur_var = var;
>
> @@ -464,7 +470,14 @@ static int eval_string(struct json_call *call, struct blob_buf *buf, const char
> cur_len = end - str;
> }
>
> - dest = blobmsg_realloc_string_buffer(buf, len + cur_len + 1);
> + new = blobmsg_realloc_string_buffer(buf, len + cur_len + 1);
> + if (!new) {
> + /* Make eval_string return -1 */
> + var = true;
> + break;
> + }
> +
> + dest = new;
> memcpy(dest + len, cur, cur_len);
> len += cur_len;
> }
> diff --git a/kvlist.c b/kvlist.c
> index e0a8acb..a7b6ea0 100644
> --- a/kvlist.c
> +++ b/kvlist.c
> @@ -71,20 +71,25 @@ bool kvlist_delete(struct kvlist *kv, const char *name)
> return !!node;
> }
>
> -void kvlist_set(struct kvlist *kv, const char *name, const void *data)
> +bool kvlist_set(struct kvlist *kv, const char *name, const void *data)
> {
> struct kvlist_node *node;
> char *name_buf;
> int len = kv->get_len(kv, data);
>
> - kvlist_delete(kv, name);
> -
> node = calloc_a(sizeof(struct kvlist_node) + len,
> &name_buf, strlen(name) + 1);
> + if (!node)
> + return false;
> +
> + kvlist_delete(kv, name);
> +
> memcpy(node->data, data, len);
>
> node->avl.key = strcpy(name_buf, name);
> avl_insert(&kv->avl, &node->avl);
> +
> + return true;
> }
>
> void kvlist_free(struct kvlist *kv)
> diff --git a/kvlist.h b/kvlist.h
> index 0d35b46..d59ff9e 100644
> --- a/kvlist.h
> +++ b/kvlist.h
> @@ -45,7 +45,7 @@ struct kvlist_node {
> void kvlist_init(struct kvlist *kv, int (*get_len)(struct kvlist *kv, const void *data));
> void kvlist_free(struct kvlist *kv);
> void *kvlist_get(struct kvlist *kv, const char *name);
> -void kvlist_set(struct kvlist *kv, const char *name, const void *data);
> +bool kvlist_set(struct kvlist *kv, const char *name, const void *data);
> bool kvlist_delete(struct kvlist *kv, const char *name);
>
> int kvlist_strlen(struct kvlist *kv, const void *data);
> diff --git a/lua/uloop.c b/lua/uloop.c
> index 782b5a5..db89e72 100644
> --- a/lua/uloop.c
> +++ b/lua/uloop.c
> @@ -325,9 +325,12 @@ static int ul_process(lua_State *L)
> int argn = lua_objlen(L, -3);
> int envn = lua_objlen(L, -2);
> char** argp = malloc(sizeof(char*) * (argn + 2));
> - char** envp = malloc(sizeof(char*) * envn + 1);
> + char** envp = malloc(sizeof(char*) * (envn + 1));
> int i = 1;
>
> + if (!argp || !envp)
> + exit(-1);
A "return luaL_error(L, "Out of memory");" might be slightly more
appropriate here.
> +
> argp[0] = (char*) lua_tostring(L, -4);
> for (i = 1; i <= argn; i++) {
> lua_rawgeti(L, -3, i);
> diff --git a/ustream.c b/ustream.c
> index e7ee9f0..d36ce08 100644
> --- a/ustream.c
> +++ b/ustream.c
> @@ -65,6 +65,9 @@ static int ustream_alloc_default(struct ustream *s, struct ustream_buf_list *l)
> return -1;
>
> buf = malloc(sizeof(*buf) + l->buffer_len + s->string_data);
> + if (!buf)
> + return -1;
> +
> ustream_init_buf(buf, l->buffer_len);
> ustream_add_buf(l, buf);
>
> @@ -490,6 +493,8 @@ int ustream_vprintf(struct ustream *s, const char *format, va_list arg)
> return ustream_write_buffered(s, buf, maxlen, wr);
> } else {
> buf = malloc(maxlen + 1);
> + if (!buf)
> + return 0;
> wr = vsnprintf(buf, maxlen + 1, format, arg);
> wr = ustream_write(s, buf, wr, false);
> free(buf);
> @@ -517,6 +522,8 @@ int ustream_vprintf(struct ustream *s, const char *format, va_list arg)
> return wr;
>
> buf = malloc(maxlen + 1);
> + if (!buf)
> + return wr;
> maxlen = vsnprintf(buf, maxlen + 1, format, arg);
> wr = ustream_write_buffered(s, buf + wr, maxlen - wr, wr);
> free(buf);
> diff --git a/utils.c b/utils.c
> index 8fd19f4..91dd71e 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -43,6 +43,8 @@ void *__calloc_a(size_t len, ...)
> va_end(ap1);
>
> ptr = calloc(1, alloc_len);
> + if (!ptr)
> + return NULL;
> alloc_len = 0;
> foreach_arg(ap, cur_addr, cur_len, &ret, len) {
> *cur_addr = &ptr[alloc_len];
>
More information about the Lede-dev
mailing list