Hi again (!),
I figured out that this will not work on architectures where
sizeof(long int) != 8 and/or sizeof(size_t) != 8, i386 for example.
The *next* patch makes sure that numbers passed to malloc() are not
overflowing size_t, and also uses *unsigned long long int* everywhere
which is guaranteed to be at least 64bit. Tested on both amd64 and
i386.
Tomasz
From: Tomasz Buchert <tomasz.buch...@inria.fr>
Date: Sun, 1 Mar 2015 20:27:29 +0100
Subject: Fix multiple integer overflows.
Dimensions of a BMP file are signed, 4-byte integers. Therefore the size
of the image may be bigger than range of (int). This is fixed in
bitmap.h, by casting all offsets to unsigned long long int (and fixing
another possible overflow in bm_new).
In bitmap_io.c we make sure that width and height of the image are
non-negative and in (int) range, because other functions require it.
Moreover, we make sure that allocations do not overflow the range of
size_t by having a wrapper (safe_malloc) that tests whether the
allocation size fits in size_t.
---
src/bitmap.h | 35 +++++++++++++++++++++++++++--------
src/bitmap_io.c | 30 +++++++++++++++++++++++++++---
2 files changed, 54 insertions(+), 11 deletions(-)
diff --git a/src/bitmap.h b/src/bitmap.h
index 1ce13d6..7110926 100644
--- a/src/bitmap.h
+++ b/src/bitmap.h
@@ -7,6 +7,7 @@
#include <string.h>
#include <stdlib.h>
+#include <errno.h>
/* The bitmap type is defined in potracelib.h */
#include "potracelib.h"
@@ -27,7 +28,10 @@
/* macros for accessing pixel at index (x,y). U* macros omit the
bounds check. */
-#define bm_scanline(bm, y) ((bm)->map + (y)*(bm)->dy)
+typedef unsigned long long int ulli;
+
+#define bm_allocsize(bm) ((ulli)(bm)->dy * (ulli)(bm)->h)
+#define bm_scanline(bm, y) ((bm)->map + ((ulli)(y))*(ulli)(bm)->dy)
#define bm_index(bm, x, y) (&bm_scanline(bm, y)[(x)/BM_WORDBITS])
#define bm_mask(x) (BM_HIBIT >> ((x) & (BM_WORDBITS-1)))
#define bm_range(x, a) ((int)(x) >= 0 && (int)(x) < (a))
@@ -43,6 +47,16 @@
#define BM_INV(bm, x, y) (bm_safe(bm, x, y) ? BM_UINV(bm, x, y) : 0)
#define BM_PUT(bm, x, y, b) (bm_safe(bm, x, y) ? BM_UPUT(bm, x, y, b) : 0)
+/* allocates memory safely */
+static inline void* safe_malloc(ulli size) {
+ size_t maxsize = (size_t)-1;
+ if (size > maxsize) {
+ errno = ENOMEM;
+ return NULL;
+ }
+ return malloc((size_t)size);
+}
+
/* free the given bitmap. Leaves errno untouched. */
static inline void bm_free(potrace_bitmap_t *bm) {
if (bm) {
@@ -54,16 +68,21 @@ static inline void bm_free(potrace_bitmap_t *bm) {
/* return new un-initialized bitmap. NULL with errno on error */
static inline potrace_bitmap_t *bm_new(int w, int h) {
potrace_bitmap_t *bm;
- int dy = (w + BM_WORDBITS - 1) / BM_WORDBITS;
+ int dy;
+
+ if (w % BM_WORDBITS == 0)
+ dy = w / BM_WORDBITS;
+ else
+ dy = (w / BM_WORDBITS) + 1;
- bm = (potrace_bitmap_t *) malloc(sizeof(potrace_bitmap_t));
+ bm = (potrace_bitmap_t *)safe_malloc(sizeof(potrace_bitmap_t));
if (!bm) {
return NULL;
}
bm->w = w;
bm->h = h;
bm->dy = dy;
- bm->map = (potrace_word *) malloc(dy * h * BM_WORDSIZE);
+ bm->map = (potrace_word *)safe_malloc(bm_allocsize(bm) * BM_WORDSIZE);
if (!bm->map) {
free(bm);
return NULL;
@@ -73,7 +92,7 @@ static inline potrace_bitmap_t *bm_new(int w, int h) {
/* clear the given bitmap. Set all bits to c. */
static inline void bm_clear(potrace_bitmap_t *bm, int c) {
- memset(bm->map, c ? -1 : 0, bm->dy * bm->h * BM_WORDSIZE);
+ memset(bm->map, c ? -1 : 0, bm_allocsize(bm) * BM_WORDSIZE);
}
/* duplicate the given bitmap. Return NULL on error with errno set. */
@@ -82,14 +101,14 @@ static inline potrace_bitmap_t *bm_dup(const potrace_bitmap_t *bm) {
if (!bm1) {
return NULL;
}
- memcpy(bm1->map, bm->map, bm->dy * bm->h * BM_WORDSIZE);
+ memcpy(bm1->map, bm->map, bm_allocsize(bm) * BM_WORDSIZE);
return bm1;
}
/* invert the given bitmap. */
static inline void bm_invert(potrace_bitmap_t *bm) {
- int i;
- for (i = 0; i < bm->dy * bm->h; i++) {
+ ulli i;
+ for (i = 0; i < bm_allocsize(bm); i++) {
bm->map[i] ^= BM_ALLBITS;
}
}
diff --git a/src/bitmap_io.c b/src/bitmap_io.c
index 6cfecb1..ea2d188 100644
--- a/src/bitmap_io.c
+++ b/src/bitmap_io.c
@@ -381,6 +381,16 @@ static int bmp_readint(FILE *f, int n, unsigned int *p) {
return 0;
}
+/* converts unsigned to signed using 32 bits */
+static int unsigned_to_signed32(unsigned int x) {
+ unsigned int sign = 0x80000000U;
+ unsigned int mask = 0xFFFFFFFFU;
+ if (sign & x)
+ return -(int)(x ^ mask) - 1;
+ else
+ return (int)x;
+}
+
/* reset padding boundary */
static void bmp_pad_reset(void) {
bmp_count = 0;
@@ -478,12 +488,25 @@ static int bm_readbody_bmp(FILE *f, double threshold, potrace_bitmap_t **bmp) {
TRY(bmp_readint(f, 4, &bmpinfo.BlueMask));
TRY(bmp_readint(f, 4, &bmpinfo.AlphaMask));
}
- if ((signed int)bmpinfo.h < 0) {
- bmpinfo.h = -bmpinfo.h;
+ int maxdim = 0x7ffffffe; /* 2^31 - 2 */
+ int sign_h = unsigned_to_signed32(bmpinfo.h);
+ int sign_w = unsigned_to_signed32(bmpinfo.w);
+ if (sign_w < 0 || sign_w > maxdim) {
+ bm_read_error = "incorrect bmp width";
+ goto format_error;
+ }
+ if (sign_h < -maxdim || sign_h > maxdim) {
+ bm_read_error = "incorrect bmp height";
+ goto format_error;
+ }
+ if (sign_h < 0) {
+ bmpinfo.h = (unsigned int)(-sign_h);
bmpinfo.topdown = 1;
} else {
bmpinfo.topdown = 0;
}
+ /* now we know that bmpinfo.{w,h} are non-negative ints
+ that fit in int type (cf. bm_new)) */
} else if (bmpinfo.InfoSize == 12) {
/* old OS/2 format */
bmpinfo.ctbits = 24; /* sample size in color table */
@@ -517,7 +540,8 @@ static int bm_readbody_bmp(FILE *f, double threshold, potrace_bitmap_t **bmp) {
/* color table, present only if bmpinfo.bits <= 8. */
if (bmpinfo.bits <= 8) {
- coltable = (int *) malloc(bmpinfo.ncolors * sizeof(int));
+ ulli bytes = (ulli)bmpinfo.ncolors * (ulli)sizeof(int);
+ coltable = (int *)safe_malloc(bytes);
if (!coltable) {
goto std_error;
}
