Coverity cleanups

2015-10-03 Thread Dirk Hohndel 
As usual before a release I spent a day trying to clean out all their
warnings. A lot of them were clearly false positives, a lot of them were
somewhat questionable but I could see at least the theoretical point and
added some more checks. And a few real bugs were found.

I ignored this last time around (bad me) but this time I tried to clean up
the datatrac.c file which ignored all return values from fread and fseek.
The code now in master should be a lot more robust when given bogus data
files - but I would love it if Salvador could check that this still
actually works. It would be good if we had a test case for this, too.

My plan is still to release Beta 3 tomorrow... as usual, more testing (and
sending of patches / bug fixes) until then is very appreciated.

/D

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

[PATCH] Clean up Divesoft Freedome time parsing

2015-10-03 Thread Linus Torvalds 

From: Linus Torvalds 
Date: Sat, 3 Oct 2015 10:29:40 -0400
Subject: [PATCH] Clean up Divesoft Freedome time parsing

So Anton Lundin says that the 32-bit timestamp for the Divesoft Freedom
is indeed a signed offset from Jan 1, 2000.

This does that, but also extracts the whole thing into a helper function
and makes sure that there are no overflows at any point by using
"timestamp_t" in the whole series, and all the operations are "obviously
safe" in their types (ie no "unsigned char gets widened to 'int' and
then we shift it left by 24 bits").

Signed-off-by: Linus Torvalds 
---

This is a bigger patch than required (it could have just changed the 
"uint32_t" into "int32_t"), but I think the code ends up being easier to 
read, and it should be as efficient (although because we compile with 
debugging and no optimizations, splitting things up like this ends up 
making the compiler split things up too - but that's our debug thing, 
not anything fundamental).

Also note that I didn't actually *test* this. Obviously. I don't  have a 
Divesoft divecomputer, much less anything that hass odd date offsets.

 parse-xml.c | 25 +++--
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/parse-xml.c b/parse-xml.c
index 1aaeb42fe33e..b2bc76f0c159 100644
--- a/parse-xml.c
+++ b/parse-xml.c
@@ -3246,6 +3246,24 @@ int parse_divinglog_buffer(sqlite3 *handle, const char 
*url, const char *buffer,
return 0;
 }
 
+/*
+ * Parse a signed 32-bit integer in little-endian mode,
+ * that is seconds since Jan 1, 2000.
+ */
+static timestamp_t parse_dlf_timestamp(unsigned char *buffer)
+{
+   timestamp_t offset;
+
+   offset = (signed char) buffer[3];
+   offset = (offset << 8) + buffer[2];
+   offset = (offset << 8) + buffer[1];
+   offset = (offset << 8) + buffer[0];
+
+   // Jan 1, 2000 is 946684800 seconds after Jan 1, 1970, which is
+   // the Unix epoch date that "timestamp_t" uses.
+   return offset + 946684800;
+}
+
 int parse_dlf_buffer(unsigned char *buffer, size_t size)
 {
unsigned char *ptr = buffer;
@@ -3268,12 +3286,7 @@ int parse_dlf_buffer(unsigned char *buffer, size_t size)
// (ptr[7] << 8) + ptr[6] Is "Serial"
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
-   // ("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 = (timestamp_t) offset + 946684800;
+   cur_dc->when = parse_dlf_timestamp(ptr + 8);
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] Clean up Divesoft Freedome time parsing

2015-10-03 Thread Linus Torvalds 
On Oct 3, 2015 11:06, "Anton Lundin"  wrote:
>
> I looked at the wrong typedef and its a uint32_t. Sorry for the mess.

Christ. Oh well, fixing up the comment and removing the cast to (signed
char) would just fix that.

Dirk, did you already apply it? Just edit the patch if not.

 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

2015-10-03 Thread 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

2015-10-03 Thread 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] Clean up Divesoft Freedome time parsing

2015-10-03 Thread Linus Torvalds 
On Oct 3, 2015 11:38 AM, "Anton Lundin"  wrote:
>
> It ain't that easy trying to read code and write email while being a
> human climbing tree and trying to keep the little one from eating all
> every cable.

Well, it's unlikely to be a problem. At least for a while. If it's actually
unsigned, we won't be seeing the high bit being set until the year 2068.

So we have a few years to get this right.

 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

2015-10-03 Thread 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] Clean up Divesoft Freedome time parsing

2015-10-03 Thread Dirk Hohndel 
On Sat, Oct 03, 2015 at 10:36:11AM -0400, Linus Torvalds wrote:
> 
> This is a bigger patch than required (it could have just changed the 
> "uint32_t" into "int32_t"), but I think the code ends up being easier to 
> read, and it should be as efficient (although because we compile with 
> debugging and no optimizations, splitting things up like this ends up 
> making the compiler split things up too - but that's our debug thing, 
> not anything fundamental).

I'm obviously not one bit worried about the overhead, but I actually like
this much better as it is clean and obvious.

Thanks, Linus

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

Re: [PATCH] Clean up Divesoft Freedome time parsing

2015-10-03 Thread Anton Lundin 
On 03 October, 2015 - Linus Torvalds wrote:

> On Oct 3, 2015 11:06, "Anton Lundin"  wrote:
> >
> > I looked at the wrong typedef and its a uint32_t. Sorry for the mess.
> 
> Christ.

Don't involve your imaginary friends in this =)

It ain't that easy trying to read code and write email while being a
human climbing tree and trying to keep the little one from eating all
every cable.

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

2015-10-03 Thread 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] Clean up Divesoft Freedome time parsing

2015-10-03 Thread Anton Lundin 
On 03 October, 2015 - Linus Torvalds wrote:

> 
> From: Linus Torvalds 
> Date: Sat, 3 Oct 2015 10:29:40 -0400
> Subject: [PATCH] Clean up Divesoft Freedome time parsing
> 
> So Anton Lundin says that the 32-bit timestamp for the Divesoft Freedom
> is indeed a signed offset from Jan 1, 2000.
> 
> This does that, but also extracts the whole thing into a helper function
> and makes sure that there are no overflows at any point by using
> "timestamp_t" in the whole series, and all the operations are "obviously
> safe" in their types (ie no "unsigned char gets widened to 'int' and
> then we shift it left by 24 bits").
> 

I looked at the wrong typedef and its a uint32_t. Sorry for the mess.


//Anton


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

[PATCH 1/2] Revert "VPM-B restore deco state before calculating next gradient"

2015-10-03 Thread Rick Walsh 
This reverts commit 3d8e5b638ad4c1fbb43f6dd5f535bf0b33a51f0b.

Calculating the next gradient should be based on the tissue loading at the end
of the previous iteration, so it was wrong to restore the deco state first.
This has a tiny affect on the calculated profile, and makes one of the tests
fail.
---
 planner.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/planner.c b/planner.c
index 4a2fb34..05fd26e 100644
--- a/planner.c
+++ b/planner.c
@@ -1140,11 +1140,11 @@ bool plan(struct diveplan *diveplan, char 
**cached_datap, bool is_planner, bool
//CVA
do {
is_final_plan = (prefs.deco_mode == BUEHLMANN) || 
(previous_deco_time - deco_time < 10);  // CVA time converges
-   restore_deco_state(bottom_cache);
if (deco_time != 1000)
vpmb_next_gradient(deco_time, 
diveplan->surface_pressure / 1000.0);
 
previous_deco_time = deco_time;
+   restore_deco_state(bottom_cache);
 
depth = bottom_depth;
gi = bottom_gi;
-- 
2.4.3

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

[PATCH 2/2] VPM-B: Adjust Subsurface conservatism

2015-10-03 Thread Rick Walsh 
Reverting commit 3d8e5b638ad4c1fbb43f6dd5f535bf0b33a51f0b makes the CVA
calculation marginally less conservative, and one of the tests fails as a
result.  This tiny adjustment to the conservatism fixes that.

Signed-off-by: Rick Walsh 
---
 deco.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/deco.c b/deco.c
index f9d6032..86acc03 100644
--- a/deco.c
+++ b/deco.c
@@ -24,8 +24,8 @@
 #define cube(x) (x * x * x)
 
 // Subsurface appears to produce marginally less conservative plans than our 
benchmarks
-// Introduce 1% additional conservatism
-#define subsurface_conservatism_factor 1.01
+// Introduce 1.2% additional conservatism
+#define subsurface_conservatism_factor 1.012
 
 
 extern bool in_planner();
-- 
2.4.3

___
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

2015-10-03 Thread 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