From b8bb38ec763fd358795fbf478510410f01fa4d09 Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Sun, 18 Feb 2024 17:59:04 -0800 Subject: [PATCH] safety: refactor iffoutput.cpp for memory safety (#4144) Static analysis has been yelling about code in iffoutput.cpp possibly overrunning buffers, and frankly the code in this module is so confusing that I can't tell if it's correct or not. (It's 13 years old, hasn't been touched for a long time.) In an ideal world, this would all be rewritten to use spans so the buffer lengths are known and checked. But I don't really have the time or inclination to rewrite it -- after all, iff is not a very important file format, though I do think it's used enough that we can't drop it entirely. The payoff from a full rewrite is marginal. So I came up with the following compromise, embodied by this PR: I'm making spans of the regions we're ultimately reading from and writing to, passing those down the chain of function calls, and so even though the actual operations are total pointer arithmetic spaghetti, we can use those spans to verify that the pointers are still within the span bounds any time we read or write through them. We do it with assertions that are only active for debug builds, so there shouldn't be any measurable performance hit. But the assertions will be active in CI for both the static and dynamic analysis tests, so should catch any latent bugs. Signed-off-by: Larry Gritz --- src/iff.imageio/iffoutput.cpp | 48 ++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/src/iff.imageio/iffoutput.cpp b/src/iff.imageio/iffoutput.cpp index 5cf4b6a222..a635a47765 100644 --- a/src/iff.imageio/iffoutput.cpp +++ b/src/iff.imageio/iffoutput.cpp @@ -83,13 +83,16 @@ class IffOutput final : public ImageOutput { } // helper to compress verbatim - void compress_verbatim(const uint8_t*& in, uint8_t*& out, int size); + void compress_verbatim(const uint8_t*& in, uint8_t*& out, int size, + cspan in_span, span out_span); // helper to compress duplicate - void compress_duplicate(const uint8_t*& in, uint8_t*& out, int size); + void compress_duplicate(const uint8_t*& in, uint8_t*& out, int size, + cspan in_span, span out_span); // helper to compress a rle channel - size_t compress_rle_channel(const uint8_t* in, uint8_t* out, int size); + size_t compress_rle_channel(const uint8_t* in, uint8_t* out, int size, + cspan in_span, span out_span); }; @@ -352,15 +355,15 @@ IffOutput::close(void) int bytespp = m_spec.pixel_bytes(); - std::vector flip(m_spec.width * bytespp); - unsigned char *src, *dst, *tmp = &flip[0]; + std::vector fliptmp(m_spec.width * bytespp); for (int y = 0; y < m_spec.height / 2; y++) { - src = &m_buf[(m_spec.height - y - 1) * m_spec.width * bytespp]; - dst = &m_buf[y * m_spec.width * bytespp]; + unsigned char* src + = &m_buf[(m_spec.height - y - 1) * m_spec.width * bytespp]; + unsigned char* dst = &m_buf[y * m_spec.width * bytespp]; - memcpy(tmp, src, m_spec.width * bytespp); + memcpy(fliptmp.data(), src, m_spec.width * bytespp); memcpy(src, dst, m_spec.width * bytespp); - memcpy(dst, tmp, m_spec.width * bytespp); + memcpy(dst, fliptmp.data(), m_spec.width * bytespp); } // write y-tiles @@ -403,8 +406,7 @@ IffOutput::close(void) bool tile_compress = (m_iff_header.compression == RLE); // set bytes. - std::vector scratch; - scratch.resize(tile_length); + std::vector scratch(tile_length); uint8_t* out_p = static_cast(&scratch[0]); @@ -441,7 +443,7 @@ IffOutput::close(void) // compress rle channel size = compress_rle_channel(&in[0], &tmp[0] + index, - tw * th); + tw * th, in, tmp); index += size; } @@ -551,7 +553,7 @@ IffOutput::close(void) // compress rle channel size = compress_rle_channel(&in[0], &tmp[0] + index, - tw * th); + tw * th, in, tmp); index += size; } @@ -654,8 +656,10 @@ IffOutput::close(void) void -IffOutput::compress_verbatim(const uint8_t*& in, uint8_t*& out, int size) +IffOutput::compress_verbatim(const uint8_t*& in, uint8_t*& out, int size, + cspan in_span, span out_span) { + OIIO_DASSERT(in >= in_span.cbegin() && in + size <= in_span.cend()); int count = 1; unsigned char byte = 0; @@ -671,7 +675,9 @@ IffOutput::compress_verbatim(const uint8_t*& in, uint8_t*& out, int size) } // copy + OIIO_DASSERT(out >= out_span.begin() && out < out_span.end()); *out++ = count - 1; + OIIO_DASSERT(out >= out_span.begin() && out + count <= out_span.end()); memcpy(out, in, count); out += count; @@ -681,8 +687,10 @@ IffOutput::compress_verbatim(const uint8_t*& in, uint8_t*& out, int size) void -IffOutput::compress_duplicate(const uint8_t*& in, uint8_t*& out, int size) +IffOutput::compress_duplicate(const uint8_t*& in, uint8_t*& out, int size, + cspan in_span, span out_span) { + OIIO_DASSERT(in >= in_span.cbegin() && in + size <= in_span.cend()); int count = 1; for (; count < size; ++count) { if (in[count - 1] != in[count]) @@ -693,6 +701,7 @@ IffOutput::compress_duplicate(const uint8_t*& in, uint8_t*& out, int size) const int length = run ? 1 : count; // copy + OIIO_DASSERT(out >= out_span.begin() && out + 2 <= out_span.end()); *out++ = ((count - 1) & 0x7f) | (run << 7); *out = *in; @@ -703,10 +712,13 @@ IffOutput::compress_duplicate(const uint8_t*& in, uint8_t*& out, int size) size_t -IffOutput::compress_rle_channel(const uint8_t* in, uint8_t* out, int size) +IffOutput::compress_rle_channel(const uint8_t* in, uint8_t* out, int size, + cspan in_span, span out_span) { const uint8_t* const _out = out; const uint8_t* const end = in + size; + OIIO_DASSERT(in >= in_span.cbegin() && in + size <= in_span.cend()); + OIIO_DASSERT(out >= out_span.begin() && out < out_span.end()); while (in < end) { // find runs @@ -714,10 +726,10 @@ IffOutput::compress_rle_channel(const uint8_t* in, uint8_t* out, int size) if (max > 0) { if (in < (end - 1) && in[0] == in[1]) { // compress duplicate - compress_duplicate(in, out, max); + compress_duplicate(in, out, max, in_span, out_span); } else { // compress verbatim - compress_verbatim(in, out, max); + compress_verbatim(in, out, max, in_span, out_span); } } }