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

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