[PATCH] Fix 32-bit overflow in Divesoft Freedom time handling

[Linus Torvalds]

From: Linus Torvalds <torvalds at 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 <torvalds at 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