Skip to content

Commit e086aa8

Browse files
lgritzlgritz
committed
fix(dpx): Several safety fixes for corrupt DPX files (AcademySoftwareFoundation#5170)
* SWAPRGBABytes: convert to from raw pointers to span-based. * A variety of overflow safety fixes, replace int (32 bit) with size_t or int64_t and use safe_mult64. * Use ImageInput::check_open() for dpx files to check for reasonable/legal resolutions and channel counts * Comment out more function declarations not used by OIIO. Aside: I should say, this code was originally imported/vendored from another project by Patrick Palmer: https://github.com/PatrickPalmer/dpx We imagined it might be an ongoing/improving work, so we made some very minor changes here and there but endeavoured to make as little change as possible -- even excluding it from our clang-format rules! -- so that we could diff against the changing dpx project and pull in any changes or even use it as an external dependency. But as you can see if you go there, it hasn't had any modifications for 17 years! So we never needed the "feature" of minimizing divergence from the original. And now I think with the rate of discovery and reporting of vulnerabilities and bugs accelerating, the pressure is on to make this code "safer," for example with these changes in this PR. I think it's time to give up the pretense entirely and just allow ourselves to fully absorb this code as our own, be unconcerned about divergence. So after this PR is merged, I expect follow-ons to: - Once and for all, fully remove the "dead code" parts that we commented out because they aren't used in OIIO. - Allow clang-format to process these files and bring them into formatting unity with the rest of OIIO. - Convert all the raw pointer use to spans - Remove redundant code -- functions in the original dpx project that were functionality equivalent to things already in OIIO -- where we kept the originals in place for the sake of minimizing divergence. Let's just use the OIIO ones we use everywhere else, in cases where they already exist. - Root out all remaining overflow and bounds issues, some of the new LLM based tools are really good at finding those. --------- Signed-off-by: Larry Gritz <lg@larrygritz.com>
1 parent 54491b2 commit e086aa8

3 files changed

Lines changed: 56 additions & 43 deletions

File tree

‎src/dpx.imageio/dpxinput.cpp‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,8 @@ DPXInput::seek_subimage(int subimage, int miplevel)
208208
m_spec = ImageSpec(m_dpx.header.Width(), m_dpx.header.Height(),
209209
m_dpx.header.ImageElementComponentCount(subimage),
210210
typedesc);
211+
if (!check_open(m_spec, { 0, 1 << 20, 0, 1 << 20, 0, 1 << 16, 0, 8 }))
212+
return false;
211213

212214
// xOffset/yOffset are defined as unsigned 32-bit integers, but m_spec.x/y are signed
213215
// avoid casts that would result in negative values
@@ -599,7 +601,8 @@ DPXInput::read_native_scanlines(int subimage, int miplevel, int ybegin,
599601
} else {
600602
// read the scanline and convert to RGB
601603
unsigned char* ptr = (unsigned char*)data;
602-
int bufsize = dpx::QueryRGBBufferSize(m_dpx.header, subimage, block);
604+
int64_t bufsize = dpx::QueryRGBBufferSize(m_dpx.header, subimage,
605+
block);
603606
if (bufsize > 0) {
604607
m_decodebuf.resize(bufsize);
605608
ptr = m_decodebuf.data();

‎src/dpx.imageio/libdpx/DPXColorConverter.cpp‎

Lines changed: 45 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -34,19 +34,22 @@
3434

3535

3636
#include "DPXColorConverter.h"
37+
#include <OpenImageIO/fmath.h>
38+
#include <OpenImageIO/span.h>
3739
#include <algorithm>
3840

3941
namespace dpx {
4042
template <typename DATA>
41-
static inline bool SwapRGBABytes(const DATA *input, DATA *output, int pixels) {
42-
// copy the data that could be destroyed to an additional buffer in case input == output
43-
DATA tmp[2];
44-
for (int i = 0; i < pixels; i++) {
45-
memcpy(tmp, &input[i * 4], sizeof(DATA) * 2);
46-
output[i * 4 + 0] = input[i * 4 + 3];
47-
output[i * 4 + 1] = input[i * 4 + 2];
48-
output[i * 4 + 2] = tmp[1];
49-
output[i * 4 + 3] = tmp[0];
43+
static inline bool SwapRGBABytes(OIIO::cspan<DATA> input, OIIO::span<DATA> output) {
44+
// Because we are within the lengths by definition, we can just use
45+
// data pointers below to avoid a bounds check on every one.
46+
size_t pixels = input.size() / 4;
47+
for (size_t i = 0; i < pixels; i++) {
48+
DATA a = input.data()[i * 4 + 0], b = input.data()[i * 4 + 1];
49+
output.data()[i * 4 + 0] = input.data()[i * 4 + 3];
50+
output.data()[i * 4 + 1] = input.data()[i * 4 + 2];
51+
output.data()[i * 4 + 2] = b;
52+
output.data()[i * 4 + 3] = a;
5053
}
5154
return true;
5255
}
@@ -107,12 +110,12 @@ namespace dpx {
107110

108111
// 4:4:4
109112
template <typename DATA, unsigned int max>
110-
static bool ConvertCbYCrToRGB(const Characteristic space, const DATA *input, DATA *output, const int pixels) {
113+
static bool ConvertCbYCrToRGB(const Characteristic space, const DATA *input, DATA *output, const size_t pixels) {
111114
const float *matrix = GetYCbCrToRGBColorMatrix(space);
112115
if (matrix == NULL)
113116
return false;
114117
DATA RGB[3];
115-
for (int i = 0; i < pixels; i++) {
118+
for (size_t i = 0; i < pixels; i++) {
116119
ConvertPixelYCbCrToRGB<DATA, max>(&input[i * 3], RGB, matrix);
117120
memcpy(&output[i * 3], RGB, sizeof(DATA) * 3);
118121
}
@@ -121,12 +124,12 @@ namespace dpx {
121124

122125
// 4:4:4:4
123126
template <typename DATA, unsigned int max>
124-
static bool ConvertCbYCrAToRGBA(const Characteristic space, const DATA *input, DATA *output, const int pixels) {
127+
static bool ConvertCbYCrAToRGBA(const Characteristic space, const DATA *input, DATA *output, const size_t pixels) {
125128
const float *matrix = GetYCbCrToRGBColorMatrix(space);
126129
if (matrix == NULL)
127130
return false;
128131
DATA RGBA[4];
129-
for (int i = 0; i < pixels; i++) {
132+
for (size_t i = 0; i < pixels; i++) {
130133
ConvertPixelYCbCrToRGB<DATA, max>(&input[i * 4], RGBA, matrix);
131134
RGBA[3] = input[i * 4 + 3];
132135
memcpy(&output[i * 4], RGBA, sizeof(DATA) * 4);
@@ -136,12 +139,12 @@ namespace dpx {
136139

137140
// 4:2:2
138141
template <typename DATA, unsigned int max>
139-
static bool ConvertCbYCrYToRGB(const Characteristic space, const DATA *input, DATA *output, const int pixels) {
142+
static bool ConvertCbYCrYToRGB(const Characteristic space, const DATA *input, DATA *output, const size_t pixels) {
140143
const float *matrix = GetYCbCrToRGBColorMatrix(space);
141144
if (matrix == NULL)
142145
return false;
143146
DATA CbYCr[3];
144-
for (int i = 0; i < pixels; i++) {
147+
for (size_t i = 0; i < pixels; i++) {
145148
// upsample to 4:4:4
146149
// FIXME: proper interpolation
147150
CbYCr[0] = input[(i | 1) * 2]; // Cb
@@ -155,12 +158,12 @@ namespace dpx {
155158

156159
// 4:2:2:4
157160
template <typename DATA, unsigned int max>
158-
static bool ConvertCbYACrYAToRGBA(const Characteristic space, const DATA *input, DATA *output, const int pixels) {
161+
static bool ConvertCbYACrYAToRGBA(const Characteristic space, const DATA *input, DATA *output, const size_t pixels) {
159162
const float *matrix = GetYCbCrToRGBColorMatrix(space);
160163
if (matrix == NULL)
161164
return false;
162165
DATA CbYCr[3];
163-
for (int i = 0; i < pixels; i++) {
166+
for (size_t i = 0; i < pixels; i++) {
164167
// upsample to 4:4:4
165168
// FIXME: proper interpolation
166169
CbYCr[0] = input[(i | 1) * 3]; // Cb
@@ -174,7 +177,7 @@ namespace dpx {
174177
}
175178

176179
static inline bool ConvertToRGBInternal(const Descriptor desc, const DataSize size, const Characteristic space,
177-
const void *input, void *output, const int pixels) {
180+
const void *input, void *output, const size_t pixels) {
178181
switch (desc) {
179182
// redundant calls
180183
case kRGB:
@@ -185,19 +188,19 @@ namespace dpx {
185188
case kABGR:
186189
switch (size) {
187190
case kByte:
188-
return SwapRGBABytes<U8>((const U8 *)input, (U8 *)output, pixels);
191+
return SwapRGBABytes(OIIO::cspan<U8>((const U8*)input, pixels*4), OIIO::span<U8>((U8*)output, pixels*4));
189192
case kWord:
190-
return SwapRGBABytes<U16>((const U16 *)input, (U16 *)output, pixels);
193+
return SwapRGBABytes(OIIO::cspan<U16>((const U16*)input, pixels*4), OIIO::span<U16>((U16*)output, pixels*4));
191194
case kInt:
192-
return SwapRGBABytes<U32>((const U32 *)input, (U32 *)output, pixels);
195+
return SwapRGBABytes(OIIO::cspan<U32>((const U32*)input, pixels*4), OIIO::span<U32>((U32*)output, pixels*4));
193196
case kFloat:
194-
return SwapRGBABytes<R32>((const R32 *)input, (R32 *)output, pixels);
197+
return SwapRGBABytes(OIIO::cspan<R32>((const R32*)input, pixels*4), OIIO::span<R32>((R32*)output, pixels*4));
195198
case kDouble:
196-
return SwapRGBABytes<R64>((const R64 *)input, (R64 *)output, pixels);
199+
return SwapRGBABytes(OIIO::cspan<R64>((const R64*)input, pixels*4), OIIO::span<R64>((R64*)output, pixels*4));
197200
}
198201
// shouldn't ever get here
199202
return false;
200-
203+
201204

202205
// FIXME: can this be translated to RGB?
203206
//case kCompositeVideo:
@@ -287,7 +290,7 @@ namespace dpx {
287290
}
288291
}
289292

290-
static inline int QueryRGBBufferSizeInternal(const Descriptor desc, const int pixels, const int bytes) {
293+
static inline int64_t QueryRGBBufferSizeInternal(const Descriptor desc, const size_t pixels, const size_t bytes) {
291294
switch (desc) {
292295
//case kCompositeVideo: // FIXME: can this be translated to RGB?
293296
case kCbYCrY: // 4:2:2 -> RGB, requires allocation
@@ -329,9 +332,10 @@ namespace dpx {
329332
}
330333
}
331334

332-
int QueryRGBBufferSize(const Header &header, const int element, const Block &block) {
335+
int64_t QueryRGBBufferSize(const Header &header, const size_t element, const Block &block) {
333336
return QueryRGBBufferSizeInternal(header.ImageDescriptor(element),
334-
(block.x2 - block.x1 + 1) * (block.y2 - block.y1 + 1),
337+
OIIO::clamped_mult64(block.x2 - block.x1 + 1,
338+
block.y2 - block.y1 + 1),
335339
header.ComponentByteCount(element));
336340
}
337341

@@ -344,10 +348,12 @@ namespace dpx {
344348
#endif /* NOT USED IN OIIO */
345349

346350
bool ConvertToRGB(const Header &header, const int element, const void *input, void *output, const Block &block) {
347-
return ConvertToRGBInternal(header.ImageDescriptor(element),
348-
header.ComponentDataSize(element), header.Colorimetric(element),
349-
input, output, (block.x2 - block.x1 + 1) * (block.y2 - block.y1 + 1));
350-
}
351+
return ConvertToRGBInternal(
352+
header.ImageDescriptor(element),
353+
header.ComponentDataSize(element), header.Colorimetric(element),
354+
input, output, OIIO::clamped_mult64(block.x2 - block.x1 + 1,
355+
block.y2 - block.y1 + 1));
356+
}
351357

352358
#if 0 /* NOT USED IN OIIO */
353359
bool ConvertToRGB(const Header &header, const int element, const void *input, void *output) {
@@ -479,7 +485,7 @@ namespace dpx {
479485
#endif /* NOT USED IN OIIO */
480486

481487
static inline bool ConvertToNativeInternal(const Descriptor desc, const DataSize size, const Characteristic space,
482-
const void *input, void *output, const int pixels) {
488+
const void *input, void *output, const size_t pixels) {
483489
switch (desc) {
484490
// redundant calls
485491
case kRGB:
@@ -491,15 +497,15 @@ namespace dpx {
491497
case kABGR:
492498
switch (size) {
493499
case kByte:
494-
return SwapRGBABytes<U8>((const U8 *)input, (U8 *)output, pixels);
500+
return SwapRGBABytes(OIIO::cspan<U8>((const U8*)input, pixels*4), OIIO::span<U8>((U8*)output, pixels*4));
495501
case kWord:
496-
return SwapRGBABytes<U16>((const U16 *)input, (U16 *)output, pixels);
502+
return SwapRGBABytes(OIIO::cspan<U16>((const U16*)input, pixels*4), OIIO::span<U16>((U16*)output, pixels*4));
497503
case kInt:
498-
return SwapRGBABytes<U32>((const U32 *)input, (U32 *)output, pixels);
504+
return SwapRGBABytes(OIIO::cspan<U32>((const U32*)input, pixels*4), OIIO::span<U32>((U32*)output, pixels*4));
499505
case kFloat:
500-
return SwapRGBABytes<R32>((const R32 *)input, (R32 *)output, pixels);
506+
return SwapRGBABytes(OIIO::cspan<R32>((const R32*)input, pixels*4), OIIO::span<R32>((R32*)output, pixels*4));
501507
case kDouble:
502-
return SwapRGBABytes<R64>((const R64 *)input, (R64 *)output, pixels);
508+
return SwapRGBABytes(OIIO::cspan<R64>((const R64*)input, pixels*4), OIIO::span<R64>((R64*)output, pixels*4));
503509
}
504510
// shouldn't ever get here
505511
return false;
@@ -647,7 +653,7 @@ namespace dpx {
647653
}
648654
#endif /* NOT USED IN OIIO */
649655

650-
bool ConvertToNative(const Descriptor desc, const DataSize compSize, const Characteristic cmetr, const int width, const int height, const void *input, void *output) {
651-
return ConvertToNativeInternal(desc, compSize, cmetr, input, output, width * height);
656+
bool ConvertToNative(const Descriptor desc, const DataSize compSize, const Characteristic cmetr, const size_t width, const size_t height, const void *input, void *output) {
657+
return ConvertToNativeInternal(desc, compSize, cmetr, input, output, OIIO::clamped_mult64(width, height));
652658
}
653659
}

‎src/dpx.imageio/libdpx/DPXColorConverter.h‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,9 @@ namespace dpx
5454
* of the buffer in bytes; sign: positive - memory needs to be allocated,
5555
* negative - allocation is optional, decoded data can replace the input
5656
*/
57-
int QueryRGBBufferSize(const Header &header, const int element, const Block &block);
57+
int64_t QueryRGBBufferSize(const Header &header, const size_t element, const Block &block);
5858

59+
#if 0 /* NOT USED IN OIIO */
5960
/*!
6061
* \brief Query the size of the buffer necessary to hold the decoded RGB data
6162
* \param header DPX header
@@ -64,7 +65,8 @@ namespace dpx
6465
* of the buffer in bytes; sign: positive - memory needs to be allocated,
6566
* negative - allocation is optional, decoded data can replace the input
6667
*/
67-
int QueryRGBBufferSize(const Header &header, const int element);
68+
int QueryRGBBufferSize(const Header &header, const size_t element);
69+
#endif
6870

6971
/*!
7072
* \brief Convert native data from the input buffer into RGB in the output buffer
@@ -78,6 +80,7 @@ namespace dpx
7880
*/
7981
bool ConvertToRGB(const Header &header, const int element, const void *input, void *output, const Block &block);
8082

83+
#if 0 /* NOT USED IN OIIO */
8184
/*!
8285
* \brief Convert native data from the input buffer into RGB in the output buffer
8386
* \param header DPX header
@@ -122,6 +125,7 @@ namespace dpx
122125
* \return success true/false
123126
*/
124127
bool ConvertToNative(const Descriptor desc, const DataSize compSize, const Characteristic cmetr, const void *input, void *output, const Block &block);
128+
#endif
125129

126130
/*!
127131
* \brief Convert RGB data from the input buffer into native format in the output buffer
@@ -132,7 +136,7 @@ namespace dpx
132136
* \param output output buffer data; can be same as input if \ref QueryNativeBufferSize returns a negative number
133137
* \return success true/false
134138
*/
135-
bool ConvertToNative(const Descriptor desc, const DataSize compSize, const Characteristic cmetr, const int width, const int height, const void *input, void *output);
139+
bool ConvertToNative(const Descriptor desc, const DataSize compSize, const Characteristic cmetr, const size_t width, const size_t height, const void *input, void *output);
136140

137141
}
138142

0 commit comments

Comments
 (0)