[OpenWrt-Devel] [LEDE-DEV] [PATCH libubox 1/3] Fix various memory management issues

Jo-Philipp Wich jo at mein.io
Tue Jun 21 06:32:48 EDT 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];
> 
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel


More information about the openwrt-devel mailing list