fix(quicktimevideo) avoid out of bounds read, closes #2340 #2341
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2341 +/- ##
==========================================
+ Coverage 63.16% 63.38% +0.21%
==========================================
Files 119 119
Lines 20433 20621 +188
Branches 10164 10231 +67
==========================================
+ Hits 12907 13071 +164
- Misses 5403 5420 +17
- Partials 2123 2130 +7
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| @@ -668,7 +668,7 @@ void QuickTimeVideo::previewTagDecoder(size_t size) { | |||
| if (equalsQTimeTag(buf, "PICT")) | |||
| xmpData_["Xmp.video.PreviewAtomType"] = "QuickDraw Picture"; | |||
| else | |||
| xmpData_["Xmp.video.PreviewAtomType"] = Exiv2::toString(buf.data()); | |||
| xmpData_["Xmp.video.PreviewAtomType"] = std::string{buf.c_str(), 4}; | |||
There was a problem hiding this comment.
I checked the documentation on this: it will copy 4 bytes even if some of them are null. So std::string::size() will return 4 even if the string is actually shorter than that, which is a bit weird. So I wonder if it would be better to increase the size of buf to 5 and put a nul-terminator in buf[4].
There was a problem hiding this comment.
Hi Kevin. I also looked a bit in this function, and this line looked safe to me, taking into account that in line 660 the DataBuf instance is of size 4, and in line 667 we do io_->readOrThrow(buf.data(), 4);.
What it might prevent additional changes in the future would be to do io_->readOrThrow(buf.data(), buf.size()); just in case the side of the DataBuf instance changes in the future.
There was a problem hiding this comment.
@piponazo: Yes, I think this line of code is safe. The problem that I'm talking about is only a very minor thing. This is a demo of the weird thing that can happen:
#include <string>
#include <stdio.h>
int main() {
std::string s("hi\0\0\0", 4);
printf("%s %lu\n", s.c_str(), s.size());
return 0;
}It prints this:
hi 4
But you might expect it to print this:
hi 2
If we never call size() on the string then it doesn't even matter.
There was a problem hiding this comment.
I figured as a first step, this is a quick fix that fixes the out-of-bounds read.
I think what we really want is a safer utility to read strings. Maybe something like a read_string function on databuf similar to the read_uintxx functions. But that is a larger refactor that I didn't have time to implement.
|
@hassec |
No description provided.