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

[Dirk Hohndel]

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 <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
>