Message ID | ad8ab0cdf8d6148e7a514dcb90a8492fd272fe37.1466409587.git.mschiffer@universe-factory.net |
---|---|
State | Superseded |
Headers | show |
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@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]; >
On 06/21/2016 12:32 PM, Jo-Philipp Wich wrote: > 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@universe-factory.net> >> --- [...] >> @@ -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? This one is correct as is, s is a local variable, so assigning it at the end doesn't make sense. > >> + >> + return ret; >> } [...] Thanks for having a look, I'll fix up the rest. Matthias
On 06/21/2016 12:32 PM, Jo-Philipp Wich wrote: > Hi, > > first of all, thanks for putting work into that - I like the changes in > general. Have a few comments inline below. > > ~ Jo > [...] >> 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. This one is actually in the forked process. I'll change it from exit() to _exit() to avoid affecting resources used by the parent process (and also adjust the exit() a few lines below). I don't think it would be appropriate to raise a Lua error in the child process. Matthias
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; } -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; } 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); + 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; + + 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"); + 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; 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); + 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];
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@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(-)