From 1f9edb0b84fb0d6175d81cd06bc37fcedf809f86 Mon Sep 17 00:00:00 2001 From: Alexandre Perrin Date: Wed, 7 Jun 2017 19:37:14 +0200 Subject: [PATCH 1/2] Fix a potential use-after-free on quirc_resize failure. Before this patch, in the event of `q->image` reallocation success but `q->pixels` failure, `q->image` would point to a freed memory. After this patch, once quirc_resize() returns, `q->image` consistently point to a memory address that can be freed. There is still an inconsistency left in the example codepath: `q->{h,w}` would hold the "old" values, while `q->image` allocated size would be based in the requested width/height; hence the comment update about the state of the QR-code recognizer (it should only be passed to quirc_destroy()). --- lib/quirc.c | 12 +++--------- lib/quirc.h | 3 ++- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/lib/quirc.c b/lib/quirc.c index 4eabe81..2c6192f 100644 --- a/lib/quirc.c +++ b/lib/quirc.c @@ -36,8 +36,7 @@ struct quirc *quirc_new(void) void quirc_destroy(struct quirc *q) { - if (q->image) - free(q->image); + free(q->image); if (sizeof(*q->image) != sizeof(*q->pixels)) free(q->pixels); @@ -47,23 +46,18 @@ void quirc_destroy(struct quirc *q) int quirc_resize(struct quirc *q, int w, int h) { uint8_t *new_image = realloc(q->image, w * h); - if (!new_image) return -1; + q->image = new_image; if (sizeof(*q->image) != sizeof(*q->pixels)) { size_t new_size = w * h * sizeof(quirc_pixel_t); quirc_pixel_t *new_pixels = realloc(q->pixels, new_size); - - if (!new_pixels) { - free(new_image); + if (!new_pixels) return -1; - } - q->pixels = new_pixels; } - q->image = new_image; q->w = w; q->h = h; diff --git a/lib/quirc.h b/lib/quirc.h index 0e7cb94..47adf4b 100644 --- a/lib/quirc.h +++ b/lib/quirc.h @@ -40,7 +40,8 @@ void quirc_destroy(struct quirc *q); * specified before codes can be analyzed. * * This function returns 0 on success, or -1 if sufficient memory could - * not be allocated. + * not be allocated. On failure the QR-code recognizer should not be + * used and is expected to be given to quirc_destroy() for cleanup. */ int quirc_resize(struct quirc *q, int w, int h); From 971c9d4e8cc65257fd64cdac8db29460a1dec542 Mon Sep 17 00:00:00 2001 From: Alexandre Perrin Date: Fri, 9 Jun 2017 22:12:06 +0200 Subject: [PATCH 2/2] refactor quirc_resize So that the given quirc struct is never invalid once we return to the caller. --- lib/quirc.c | 58 ++++++++++++++++++++++++++++++++++++++++++++--------- lib/quirc.h | 3 +-- 2 files changed, 50 insertions(+), 11 deletions(-) diff --git a/lib/quirc.c b/lib/quirc.c index 2c6192f..a9da85d 100644 --- a/lib/quirc.c +++ b/lib/quirc.c @@ -45,23 +45,63 @@ void quirc_destroy(struct quirc *q) int quirc_resize(struct quirc *q, int w, int h) { - uint8_t *new_image = realloc(q->image, w * h); - if (!new_image) - return -1; - q->image = new_image; + uint8_t *image = NULL; + quirc_pixel_t *pixels = NULL; + /* + * XXX: w and h should be size_t (or at least unsigned) as negatives + * values would not make much sense. The downside is that it would break + * both the API and ABI. Thus, at the moment, let's just do a sanity + * check. + */ + if (w < 0 || h < 0) + goto fail; + + /* + * alloc a new buffer for q->image. We avoid realloc(3) because we want + * on failure to be leave `q` in a consistant, unmodified state. + */ + image = calloc(w, h); + if (!image) + goto fail; + + /* compute the "old" (i.e. currently allocated) and the "new" + (i.e. requested) image dimensions */ + size_t olddim = q->w * q->h; + size_t newdim = w * h; + size_t min = (olddim < newdim ? olddim : newdim); + + /* + * copy the data into the new buffer, avoiding (a) to read beyond the + * old buffer when the new size is greater and (b) to write beyond the + * new buffer when the new size is smaller, hence the min computation. + */ + (void)memcpy(image, q->image, min); + + /* alloc a new buffer for q->pixels if needed */ if (sizeof(*q->image) != sizeof(*q->pixels)) { - size_t new_size = w * h * sizeof(quirc_pixel_t); - quirc_pixel_t *new_pixels = realloc(q->pixels, new_size); - if (!new_pixels) - return -1; - q->pixels = new_pixels; + pixels = calloc(newdim, sizeof(quirc_pixel_t)); + if (!pixels) + goto fail; } + /* alloc succeeded, update `q` with the new size and buffers */ q->w = w; q->h = h; + free(q->image); + q->image = image; + if (sizeof(*q->image) != sizeof(*q->pixels)) { + free(q->pixels); + q->pixels = pixels; + } return 0; + /* NOTREACHED */ +fail: + free(image); + free(pixels); + + return -1; } int quirc_count(const struct quirc *q) diff --git a/lib/quirc.h b/lib/quirc.h index 47adf4b..0e7cb94 100644 --- a/lib/quirc.h +++ b/lib/quirc.h @@ -40,8 +40,7 @@ void quirc_destroy(struct quirc *q); * specified before codes can be analyzed. * * This function returns 0 on success, or -1 if sufficient memory could - * not be allocated. On failure the QR-code recognizer should not be - * used and is expected to be given to quirc_destroy() for cleanup. + * not be allocated. */ int quirc_resize(struct quirc *q, int w, int h);