I am so glad someone is keeping me honest. Thank you, Linus.
Yes, I completely misunderstood what coverity was trying to tell me. Oops. I mainly want to clean out all the noise from the Coverity reports so they become more useful. And while I ignored as "false positive" a ton of crap where it was obviously just crap - a few of them seemed reasonable enough to try and fix them (and I hope I got the other fixes right). And of course it did find a couple of bugs that we had missed so far. Again, thanks /D On Fri, Oct 02, 2015 at 09:57:04PM -0400, Linus Torvalds wrote: > > From: Linus Torvalds <torva...@linux-foundation.org> > Date: Fri, 2 Oct 2015 21:26:32 -0400 > Subject: [PATCH] Fix 32-bit overflow in Divesoft Freedom time handling > > Commit 31fb2e4c62ab ("Avoid possible sign extension") handled the > problem when a "unsigned char" is shifted 24 bits left, and becomes a > "signed int". By casting the result to uint32_t, that signed case won't > happen. > > However, there were two bugs in that fix. > > The first is the comment. It's not that "timestamp_t" is signed that is > the problem. No, the problem is inherent in the C expression > > (ptr[11] << 24) > > where "ptr[11]" is an unsigned char. In C arithmetic, unsigned char is > implicitly type-expanded to "int", so while it has a value between > 0..255, when you shift it left by 24, you can get a *negative* "int" as > a result. > > So it's actually "ptr[11]" that should have been cast to "unsigned", but > it so happens that you can do all the shifting and adding in "int", and > then cast the end result to "uint32_t" and you'll get the same value. > But at no point did "timestamp_t" matter. > > The other bug was pre-existing and just not fixed. When the code does > the "+ 946684800" (to turn the timestamp to be seconds from the start of > 2000, into seconds since the "unix epoch", ie 1970) that arithmetic is > now done in that "uint32_t" (and used to be done in "int"). > > Which means that the addition can overflow in 32 bits *before* it is > cast to timestamp_t (which is 64 bits). > > Admittedly that 32-bit overflow happens a bit later than the sign bit > gets set, but if we're worried aboout overflows, let's just do this > right. > > In other words, we have a 32-bit unsigned offset since Jan 1, 2000, and > for the full range we need to do the epoch correction in 32 bits. > Because otherwise you fail in the year 2106 (32-bit unsigned unix epoch > time limit), even though the 32-bit seconds *should* work all the way > until the year 2136. > > Of course, I'll be rather surprised if people still use the Divesoft > Freedom in the year 2106. Or rather, I won't be surprised, because I'll > be dead. > > But if we think that the signed problem matters (in the year 2068), then > dammit, we can extend it another 30 years. > > Signed-off-by: Linus Torvalds <torva...@linux-foundation.org> > --- > > This patch is entirely untested, and completely unnecessary. But it came > about when I was looking at the recent commits, and went "Hmm, that's not > right". The comment in particular is just confusingly wrong. > > Of course, it's not at all clear that the 32-bit number is actually > unsigned to begin with. Maybe it's meant to be signed, the way > traditional 32-bit unix time_t is. Maybe the Divesoft Freedom was > designed to also be able to import dives from before Jan 1, 2000. Who > knows? Not me. I've never seen one of those things. > > Even so, the addition of "946684800" (to correct between unix epoch and an > epoch based on Jan 1, 2000) should be done in "timestamp_t". > > In other words, this patch is silly. But the comment really is wrong, and > misses what Coverity was actually complaining about, and the code gets the > right answer (well, until 2106) almost by accident. > > parse-xml.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/parse-xml.c b/parse-xml.c > index 46261e9c9ef5..1aaeb42fe33e 100644 > --- a/parse-xml.c > +++ b/parse-xml.c > @@ -3269,9 +3269,11 @@ int parse_dlf_buffer(unsigned char *buffer, size_t > size) > snprintf(serial, sizeof(serial), "%d", (ptr[7] << 8) + ptr[6]); > cur_dc->serial = strdup(serial); > // Dive start time in seconds since 2000-01-01 00:00 > - // let's avoid implicit sign extensions (as timestamp_t is signed) > + // let's avoid implicit sign extensions > + // ("unsigned char" is expanded to "int" in C arithmetic, so > + // (ptr[11] << 24) can be a signed integer) > uint32_t offset = (ptr[11] << 24) + (ptr[10] << 16) + (ptr[9] << 8) + > ptr[8]; > - cur_dc->when = offset + 946684800; > + cur_dc->when = (timestamp_t) offset + 946684800; > cur_dive->when = cur_dc->when; > > cur_dc->duration.seconds = ((ptr[14] & 0xFE) << 16) + (ptr[13] << 8) + > ptr[12]; > -- > 2.6.0 > _______________________________________________ subsurface mailing list subsurface@subsurface-divelog.org http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface