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

[Dirk Hohndel]

On Sat, Oct 03, 2015 at 09:38:10AM +0200, Anton Lundin wrote:
> On 02 October, 2015 - Linus Torvalds wrote:
> 
> > 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.
> > 
> 
> The root bug was mine. Another one of C's wonderful things that i didn't
> know about.
> 
> > 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. 
> > 
> 
> Most of the work to import the Divesoft files was done by
> reverse-engineering, but after a while we got a answer from the Divesoft
> folks with a C header containing structs, enums and typedef's on how the
> format actually looks.
> 
> The tings we never managed to figure out was corrected then. One real
> gotcha was the 10-bit signed temp field in 0.1 C. I'd never managed to
> guess that it was 10-bit.
> 
> 
> Anyhow, the dive start timestamp is a 32-bit signed in seconds since
> 2000-01-01 00:00:00, according to the header file and the comments in
> it.

Which then means that the old code was actually correct and the fix and
the fix of the fix were actually wrong?

/D
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

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

[Linus Torvalds]

On Oct 3, 2015 7:03 AM, "Dirk Hohndel"  wrote:
>
> Which then means that the old code was actually correct and the fix and
> the fix of the fix were actually wrong?

Well, the final add (to convert from 2000-based numbers to 1970-based ones)
should still be done in timestamp_t. Otherwise you overflow in signed int
in 2038 (the Unix 32-bit time_t overflow date)

So I think the only thing that needs to be fixed is to change the uint32_t
to just a int32_t.

 Linus
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

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

[Anton Lundin]

On 03 October, 2015 - Anton Lundin wrote:

> On 02 October, 2015 - Linus Torvalds wrote:
> 
> > 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.
> > 
> 
> The root bug was mine. Another one of C's wonderful things that i didn't
> know about.
> 
> > 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. 
> > 
> 
> Most of the work to import the Divesoft files was done by
> reverse-engineering, but after a while we got a answer from the Divesoft
> folks with a C header containing structs, enums and typedef's on how the
> format actually looks.
> 
> The tings we never managed to figure out was corrected then. One real
> gotcha was the 10-bit signed temp field in 0.1 C. I'd never managed to
> guess that it was 10-bit.
> 
> 
> Anyhow, the dive start timestamp is a 32-bit signed in seconds since
> 2000-01-01 00:00:00, according to the header file and the comments in
> it.
> 

Fu. I looked at the wrong typedef. their time stamp is actually
uint32_t.

Sorry for the confusion.


//Anton


-- 
Anton Lundin+46702-161604
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

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

[Thiago Macieira]

On Saturday 03 October 2015 09:38:10 Anton Lundin wrote:
> The root bug was mine. Another one of C's wonderful things that i didn't
> know about.

Yeah...

unsigned short us = 0x;
unsigned u = 0x;

us * us;// this is undefined behaviour
u * u;  // this is fine

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center
  PGP/GPG: 0x6EF45358; fingerprint:
  E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358

___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

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

[Anton Lundin]

On 02 October, 2015 - Linus Torvalds wrote:

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

The root bug was mine. Another one of C's wonderful things that i didn't
know about.

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

Most of the work to import the Divesoft files was done by
reverse-engineering, but after a while we got a answer from the Divesoft
folks with a C header containing structs, enums and typedef's on how the
format actually looks.

The tings we never managed to figure out was corrected then. One real
gotcha was the 10-bit signed temp field in 0.1 C. I'd never managed to
guess that it was 10-bit.


Anyhow, the dive start timestamp is a 32-bit signed in seconds since
2000-01-01 00:00:00, according to the header file and the comments in
it.


//Anton


-- 
Anton Lundin+46702-161604
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

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

[Linus Torvalds]

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

Re: [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 <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