Bug Report
I believe I have found a bug in oldreadcolrs(...)
.
Refer to “src/common/color.c” lines 103 through 138. The function decompresses an old-style RLE-encoded scanline. The idea is that the first four bytes indicate an RGBE color (the else
block, line 131), and then a following four bytes can indicate a run using it (the if
block, line 120). The new pixel is copied from the preceding pixel (line 125).
The problem is that if the else
block doesn’t run first (e.g. because of a corrupt or maliciously constructed image), then the copycolr
macro will attempt to copy from scanline[-1]
. This will happen, for example, if the read scanline starts with bytes <1,1,1,1>. The variable rshift
is initialized to 0
, showing that it was considered what happens when the else
block doesn’t run first, but apparently it was not done so correctly.
To fix this, I recommend initializing rshift
to -1
instead, and then checking that condition inside the if
block. E.g. insert after line 122:
if (rshift<0)
return(-1); /* expected a preceding pixel to duplicate */
Missed Optimization
There is also a pretty significant missed optimization opportunity in the if
block’s test. Your current use of operator &
instead of &&
suggests performance was deemed important, so I suggest doing a single test-to-zero against a (masked) uint32, which will be much faster. E.g. for little-endian:
if ( ! (((*(uint32_t*)scanline)^0x00010101u)<<8) ) { /*...*/ }
Note that for this approach, scanline
needs to be aligned 4; which probably implies COLR
be aligned 4 (it should be anyway; this will result in performance improvements across the codebase). If it were me, I’d use something like:
typedef union {
struct { uby8 r, g, b, e; };
uby8 arr[4];
uint32_t packed;
} COLR;
—which would also make the above optimization easier.
Developer Access
A final issue for your consideration: it doesn’t seem like you provide public repo access, nor is there a location to discreetly report security issues. You even require moderator approval even to register to post on a forum. It all seems unfriendly, and designed to discourage people to help.