Skip to content

Commit 5537029

Browse files
committed
Fix #492: Potential double-free in gdImage*Ptr()
Whenever `gdImage*Ptr()` calls `gdImage*Ctx()` and the latter fails, we must not call `gdDPExtractData()`; otherwise a double-free would happen. Since `gdImage*Ctx()` are void functions, and we can't change that for BC reasons, we're introducing static helpers which are used internally. We're adding a regression test for `gdImageJpegPtr()`, but not for `gdImageGifPtr()` and `gdImageWbmpPtr()` since we don't know how to trigger failure of the respective `gdImage*Ctx()` calls. This potential security issue has been reported by Solmaz Salimi (aka. Rooney).
1 parent a414b9b commit 5537029

File tree

7 files changed

+84
-11
lines changed

7 files changed

+84
-11
lines changed

Diff for: src/gd_gif_out.c

+15-3
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,7 @@ static void char_init(GifCtx *ctx);
9999
static void char_out(int c, GifCtx *ctx);
100100
static void flush_char(GifCtx *ctx);
101101

102+
static int _gdImageGifCtx(gdImagePtr im, gdIOCtxPtr out);
102103

103104

104105

@@ -131,8 +132,11 @@ BGD_DECLARE(void *) gdImageGifPtr(gdImagePtr im, int *size)
131132
void *rv;
132133
gdIOCtx *out = gdNewDynamicCtx(2048, NULL);
133134
if (out == NULL) return NULL;
134-
gdImageGifCtx(im, out);
135-
rv = gdDPExtractData(out, size);
135+
if (!_gdImageGifCtx(im, out)) {
136+
rv = gdDPExtractData(out, size);
137+
} else {
138+
rv = NULL;
139+
}
136140
out->gd_free(out);
137141
return rv;
138142
}
@@ -220,6 +224,12 @@ BGD_DECLARE(void) gdImageGif(gdImagePtr im, FILE *outFile)
220224
221225
*/
222226
BGD_DECLARE(void) gdImageGifCtx(gdImagePtr im, gdIOCtxPtr out)
227+
{
228+
_gdImageGifCtx(im, out);
229+
}
230+
231+
/* returns 0 on success, 1 on failure */
232+
static int _gdImageGifCtx(gdImagePtr im, gdIOCtxPtr out)
223233
{
224234
gdImagePtr pim = 0, tim = im;
225235
int interlace, BitsPerPixel;
@@ -231,7 +241,7 @@ BGD_DECLARE(void) gdImageGifCtx(gdImagePtr im, gdIOCtxPtr out)
231241
based temporary image. */
232242
pim = gdImageCreatePaletteFromTrueColor(im, 1, 256);
233243
if(!pim) {
234-
return;
244+
return 1;
235245
}
236246
tim = pim;
237247
}
@@ -247,6 +257,8 @@ BGD_DECLARE(void) gdImageGifCtx(gdImagePtr im, gdIOCtxPtr out)
247257
/* Destroy palette based temporary image. */
248258
gdImageDestroy( pim);
249259
}
260+
261+
return 0;
250262
}
251263

252264

Diff for: src/gd_jpeg.c

+16-4
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,8 @@ static void fatal_jpeg_error(j_common_ptr cinfo)
117117
exit(99);
118118
}
119119

120+
static int _gdImageJpegCtx(gdImagePtr im, gdIOCtx *outfile, int quality);
121+
120122
/*
121123
* Write IM to OUTFILE as a JFIF-formatted JPEG image, using quality
122124
* QUALITY. If QUALITY is in the range 0-100, increasing values
@@ -231,8 +233,11 @@ BGD_DECLARE(void *) gdImageJpegPtr(gdImagePtr im, int *size, int quality)
231233
void *rv;
232234
gdIOCtx *out = gdNewDynamicCtx(2048, NULL);
233235
if (out == NULL) return NULL;
234-
gdImageJpegCtx(im, out, quality);
235-
rv = gdDPExtractData(out, size);
236+
if (!_gdImageJpegCtx(im, out, quality)) {
237+
rv = gdDPExtractData(out, size);
238+
} else {
239+
rv = NULL;
240+
}
236241
out->gd_free(out);
237242
return rv;
238243
}
@@ -253,6 +258,12 @@ void jpeg_gdIOCtx_dest(j_compress_ptr cinfo, gdIOCtx *outfile);
253258
254259
*/
255260
BGD_DECLARE(void) gdImageJpegCtx(gdImagePtr im, gdIOCtx *outfile, int quality)
261+
{
262+
_gdImageJpegCtx(im, outfile, quality);
263+
}
264+
265+
/* returns 0 on success, 1 on failure */
266+
static int _gdImageJpegCtx(gdImagePtr im, gdIOCtx *outfile, int quality)
256267
{
257268
struct jpeg_compress_struct cinfo;
258269
struct jpeg_error_mgr jerr;
@@ -287,7 +298,7 @@ BGD_DECLARE(void) gdImageJpegCtx(gdImagePtr im, gdIOCtx *outfile, int quality)
287298
if(row) {
288299
gdFree(row);
289300
}
290-
return;
301+
return 1;
291302
}
292303

293304
cinfo.err->emit_message = jpeg_emit_message;
@@ -328,7 +339,7 @@ BGD_DECLARE(void) gdImageJpegCtx(gdImagePtr im, gdIOCtx *outfile, int quality)
328339
if(row == 0) {
329340
gd_error("gd-jpeg: error: unable to allocate JPEG row structure: gdCalloc returns NULL\n");
330341
jpeg_destroy_compress(&cinfo);
331-
return;
342+
return 1;
332343
}
333344

334345
rowptr[0] = row;
@@ -405,6 +416,7 @@ BGD_DECLARE(void) gdImageJpegCtx(gdImagePtr im, gdIOCtx *outfile, int quality)
405416
jpeg_finish_compress(&cinfo);
406417
jpeg_destroy_compress(&cinfo);
407418
gdFree(row);
419+
return 0;
408420
}
409421

410422

Diff for: src/gd_wbmp.c

+18-3
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,8 @@ int gd_getin(void *in)
8888
return (gdGetC((gdIOCtx *)in));
8989
}
9090

91+
static int _gdImageWBMPCtx(gdImagePtr image, int fg, gdIOCtx *out);
92+
9193
/*
9294
Function: gdImageWBMPCtx
9395
@@ -100,14 +102,20 @@ int gd_getin(void *in)
100102
out - the stream where to write
101103
*/
102104
BGD_DECLARE(void) gdImageWBMPCtx(gdImagePtr image, int fg, gdIOCtx *out)
105+
{
106+
_gdImageWBMPCtx(image, fg, out);
107+
}
108+
109+
/* returns 0 on success, 1 on failure */
110+
static int _gdImageWBMPCtx(gdImagePtr image, int fg, gdIOCtx *out)
103111
{
104112
int x, y, pos;
105113
Wbmp *wbmp;
106114

107115
/* create the WBMP */
108116
if((wbmp = createwbmp(gdImageSX(image), gdImageSY(image), WBMP_WHITE)) == NULL) {
109117
gd_error("Could not create WBMP\n");
110-
return;
118+
return 1;
111119
}
112120

113121
/* fill up the WBMP structure */
@@ -123,11 +131,15 @@ BGD_DECLARE(void) gdImageWBMPCtx(gdImagePtr image, int fg, gdIOCtx *out)
123131

124132
/* write the WBMP to a gd file descriptor */
125133
if(writewbmp(wbmp, &gd_putout, out)) {
134+
freewbmp(wbmp);
126135
gd_error("Could not save WBMP\n");
136+
return 1;
127137
}
128138

129139
/* des submitted this bugfix: gdFree the memory. */
130140
freewbmp(wbmp);
141+
142+
return 0;
131143
}
132144

133145
/*
@@ -271,8 +283,11 @@ BGD_DECLARE(void *) gdImageWBMPPtr(gdImagePtr im, int *size, int fg)
271283
void *rv;
272284
gdIOCtx *out = gdNewDynamicCtx(2048, NULL);
273285
if (out == NULL) return NULL;
274-
gdImageWBMPCtx(im, fg, out);
275-
rv = gdDPExtractData(out, size);
286+
if (!_gdImageWBMPCtx(im, fg, out)) {
287+
rv = gdDPExtractData(out, size);
288+
} else {
289+
rv = NULL;
290+
}
276291
out->gd_free(out);
277292
return rv;
278293
}

Diff for: tests/jpeg/.gitignore

+1
Original file line numberDiff line numberDiff line change
@@ -3,5 +3,6 @@
33
/jpeg_empty_file
44
/jpeg_im2im
55
/jpeg_null
6+
/jpeg_ptr_double_free
67
/jpeg_read
78
/jpeg_resolution

Diff for: tests/jpeg/CMakeLists.txt

+1
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ IF(JPEG_FOUND)
22
LIST(APPEND TESTS_FILES
33
jpeg_empty_file
44
jpeg_im2im
5+
jpeg_ptr_double_free
56
jpeg_null
67
)
78

Diff for: tests/jpeg/Makemodule.am

+2-1
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,8 @@ if HAVE_LIBJPEG
22
libgd_test_programs += \
33
jpeg/jpeg_empty_file \
44
jpeg/jpeg_im2im \
5-
jpeg/jpeg_null
5+
jpeg/jpeg_null \
6+
jpeg/jpeg_ptr_double_free
67

78
if HAVE_LIBPNG
89
libgd_test_programs += \

Diff for: tests/jpeg/jpeg_ptr_double_free.c

+31
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
/**
2+
* Test that failure to convert to JPEG returns NULL
3+
*
4+
* We are creating an image, set its width to zero, and pass this image to
5+
* `gdImageJpegPtr()` which is supposed to fail, and as such should return NULL.
6+
*
7+
* See also <https://github.com/libgd/libgd/issues/381>
8+
*/
9+
10+
11+
#include "gd.h"
12+
#include "gdtest.h"
13+
14+
15+
int main()
16+
{
17+
gdImagePtr src, dst;
18+
int size;
19+
20+
src = gdImageCreateTrueColor(1, 10);
21+
gdTestAssert(src != NULL);
22+
23+
src->sx = 0; /* this hack forces gdImageJpegPtr() to fail */
24+
25+
dst = gdImageJpegPtr(src, &size, 0);
26+
gdTestAssert(dst == NULL);
27+
28+
gdImageDestroy(src);
29+
30+
return gdNumFailures();
31+
}

0 commit comments

Comments
 (0)