refactor: Use spans to solve a number of memory safety issues #4148
Conversation
* exif.cpp: use spans to avoid several possible buffer overruns
* paramlist_test.cpp: use spans to be sure we're ok on memory bounds
* jpeg: use spans in jpeg icc profile manipulation for memory safety
* psd output: switch cmyk conversion to use spans for memory safety
I've been cobbling these together bit by bit over the last several
weeks, tracking down the memory safety issues identified by the Sonar
static analysis. I think that *mostly* they are not real bugs, but the
code is confusing enough that it's hard to verify. This refactor makes
it safer, and even if the code was previously correct but merely hard
to analyze, the bounds checking in span (for debug mode) gives the
static analyzer the clues it needs to know this is safe.
As further explanation, these situations generally involve something
that looks like this, schematically:
caller () {
T buffer[known_size];
call function(buffer, ...params...);
}
function(T* buffer, ...params) {
i, j = ...very complex things, hard to analyze...
foo = buffer[i];
buffer[j] = bar;
// Did we do a buffer overrun??? ¯\_(ツ)_/¯
// This function doesn't necessarily have knowldege of the
// explicit bounds, even if it wanted to check every access.
}
By rewriting this idiom as follows:
function(span<T> buffer) {
// buffer carries its bounds, and every access will be
// checked in debug builds.
}
we are passing the bounds as well, and at least for debug builds, are
also checking bounds with an assertion on every indexed access into
buffer, without the function needing a lot of clutter. Fire and forget.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
|
Any objections? |
| if (swab) | ||
| swap_endian(d.data(), d.size()); | ||
| spec.attribute(name, type, d.data()); | ||
| cspan<uint8_t> dspan |
There was a problem hiding this comment.
Why is dspan 8-bit when it's given a 16-bit span and dswab uses it as if it was 16-bit?
There was a problem hiding this comment.
Because it's in the middle of some byte buffer, they aren't actually necessarily aligned on 16-bit boundaries. We're just copying things around, a pile of bytes is ok.
There was a problem hiding this comment.
I'll add some comments to clarify.
There was a problem hiding this comment.
Updated with better comments.
| std::vector<uint16_t> dswab((const uint16_t*)dspan.begin(), | ||
| (const uint16_t*)dspan.end()); |
There was a problem hiding this comment.
This is making a copy of dspan when previously no copy was used. Is that acceptable time and memory overhead?
There was a problem hiding this comment.
No, it's the opposite. Previously, it was ALWAYS making a copy into a vector (a few lines above). Now it only does so if it needs to mutate the data by byteswapping it. Now when there is no swap needed, there is no copy.
Signed-off-by: Larry Gritz <lg@larrygritz.com>
lgritz
merged commit 758b5a7
into
AcademySoftwareFoundation:master
I've been cobbling these together bit by bit over the last several weeks, tracking down the memory safety issues identified by the Sonar static analysis. I think that mostly they are not real bugs, but the code is confusing enough that it's hard to verify. This refactor makes it safer, and even if the code was previously correct but merely hard to analyze, the bounds checking in span (for debug mode) gives the static analyzer the clues it needs to know this is safe.
As further explanation, these situations generally involve something that looks like this, schematically:
By rewriting this function as follows:
we are passing the bounds as well, and at least for debug builds, are also checking bounds with an assertion on every indexed access into buffer, without the function needing a lot of clutter. Fire and forget.