Skip to content

Commit 4123b8e

Browse files
committed
exif/heic: Avoid undefined behaviour in address calculations
It is illegal to construct out-of-bound pointers, even if they are not dereferenced. The current bound checks rely on undefined behaviour. Fix this by introducing convenience macros that check the remaining length.
1 parent 5427120 commit 4123b8e

File tree

1 file changed

+35
-39
lines changed

1 file changed

+35
-39
lines changed

ext/exif/exif.c

Lines changed: 35 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -4311,70 +4311,63 @@ static void exif_isobmff_parse_meta(unsigned char *data, unsigned char *end, iso
43114311
unsigned char *box_offset, *p, *p2;
43124312
int header_size, exif_id = -1, version, item_count, i;
43134313

4314-
for (box_offset = data + 4; box_offset + 16 < end; box_offset += box.size) {
4314+
size_t remain;
4315+
#define CHECK(n) do { \
4316+
if (remain < (n)) { \
4317+
return; \
4318+
} \
4319+
} while (0)
4320+
#define ADVANCE(n) do { \
4321+
CHECK(n); \
4322+
remain -= (n); \
4323+
p += (n); \
4324+
} while (0)
4325+
4326+
for (box_offset = data + 4; box_offset < end - 16; box_offset += box.size) {
43154327
header_size = exif_isobmff_parse_box(box_offset, &box);
43164328
if (box.size < header_size) {
43174329
return;
43184330
}
4331+
p = box_offset;
4332+
remain = end - p;
4333+
43194334
if (box.type == FOURCC("iinf")) {
4320-
p = box_offset + header_size;
4321-
if (p >= end) {
4322-
return;
4323-
}
4324-
version = p[0];
4325-
p += 4;
4335+
ADVANCE(header_size + 4);
4336+
version = p[-4];
43264337
if (version < 2) {
4327-
if (p + 2 >= end) {
4328-
return;
4329-
}
4330-
item_count = php_ifd_get16u(p, 1);
4331-
p += 2;
4338+
ADVANCE(2);
4339+
item_count = php_ifd_get16u(p - 2, 1);
43324340
} else {
4333-
if (p + 4 >= end) {
4334-
return;
4335-
}
4336-
item_count = php_ifd_get32u(p, 1);
4337-
p += 4;
4341+
ADVANCE(4);
4342+
item_count = php_ifd_get32u(p - 4, 1);
43384343
}
4339-
for (i = 0; i < item_count && p + 20 < end; i++) {
4344+
for (i = 0; i < item_count && p < end - 20; i++) {
43404345
header_size = exif_isobmff_parse_box(p, &item);
43414346
if (item.size < header_size) {
43424347
return;
43434348
}
4344-
if (p + header_size + 12 >= end) {
4345-
return;
4346-
}
4349+
CHECK(header_size + 12);
43474350
if (!memcmp(p + header_size + 8, "Exif", 4)) {
43484351
exif_id = php_ifd_get16u(p + header_size + 4, 1);
43494352
break;
43504353
}
4351-
p += item.size;
4354+
ADVANCE(item.size);
43524355
}
43534356
if (exif_id < 0) {
43544357
break;
43554358
}
43564359
}
43574360
else if (box.type == FOURCC("iloc")) {
4358-
p = box_offset + header_size;
4359-
if (p >= end) {
4360-
return;
4361-
}
4362-
version = p[0];
4363-
p += 6;
4361+
ADVANCE(header_size + 6);
4362+
version = p[-6];
43644363
if (version < 2) {
4365-
if (p + 2 >= end) {
4366-
return;
4367-
}
4368-
item_count = php_ifd_get16u(p, 1);
4369-
p += 2;
4364+
ADVANCE(2);
4365+
item_count = php_ifd_get16u(p - 2, 1);
43704366
} else {
4371-
if (p + 4 >= end) {
4372-
return;
4373-
}
4374-
item_count = php_ifd_get32u(p, 1);
4375-
p += 4;
4367+
ADVANCE(4);
4368+
item_count = php_ifd_get32u(p - 4, 1);
43764369
}
4377-
for (i = 0, p2 = p; i < item_count && p + 16 < end; i++, p2 += 16) {
4370+
for (i = 0, p2 = p; i < item_count && p < end - 16; i++, p2 += 16) {
43784371
if (php_ifd_get16u(p2, 1) == exif_id) {
43794372
pos->offset = php_ifd_get32u(p2 + 8, 1);
43804373
pos->size = php_ifd_get32u(p2 + 12, 1);
@@ -4384,6 +4377,9 @@ static void exif_isobmff_parse_meta(unsigned char *data, unsigned char *end, iso
43844377
break;
43854378
}
43864379
}
4380+
4381+
#undef ADVANCE
4382+
#undef CHECK
43874383
}
43884384

43894385
static bool exif_scan_HEIF_header(image_info_type *ImageInfo, unsigned char *buf)

0 commit comments

Comments
 (0)