On Wed, Sep 09, 2015 at 06:13:49PM +0300, Miika Turkia wrote: > I have some trouble understanding this code, but as there is a clear > bug involved (null dereference), I ask others to verify if I am onto > something. And if datatrak import still works with this patch.
So this interesting pattern + dt_dive = NULL; + return(*dt_dive); (which Lubomir improved by removing the unnecessary parenthesis) was in the original commit 44b55bd1a220 by Salvador Cuñat <[email protected]> Me thinks that this may never have worked all that well. Salvador, any comments on Miika's patch? /D > From af2935622b1f00f373ed38c8e3194e25504372b6 Mon Sep 17 00:00:00 2001 > From: Miika Turkia <[email protected]> > Date: Wed, 9 Sep 2015 18:03:45 +0300 > Subject: [PATCH] Fix null dereference and parsing logic > > Null dereference in the first change is obviously a bug. > > The parsing logic I only assume to be wrong and suggest that we discard > dives that are deemed to be bogus. > > Signed-off-by: Miika Turkia <[email protected]> > --- > I know I should not send patches without testing them first, but I do > not have any sample data that I could use to run this through. And the > null dereference is clearly a bug, so let's see what others think of > this and if someone is able to test that parsing still works. > --- > datatrak.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/datatrak.c b/datatrak.c > index 37418c9..2e8dec8 100644 > --- a/datatrak.c > +++ b/datatrak.c > @@ -158,7 +158,7 @@ static dtrakheader read_file_header(FILE *archivo) > /* > * Parses the dive extracting its data and filling a subsurface's dive > structure > */ > -static struct dive dt_dive_parser(FILE *archivo, struct dive *dt_dive) > +bool dt_dive_parser(FILE *archivo, struct dive *dt_dive) > { > unsigned char n; > int profile_length; > @@ -185,8 +185,7 @@ static struct dive dt_dive_parser(FILE *archivo, struct > dive *dt_dive) > fread(&lector_bytes[n+1], 1, 1, archivo); > if (two_bytes_to_int(lector_bytes[0], lector_bytes[1]) != 0xA000) { > printf("Error: byte = %4x\n", two_bytes_to_int(lector_bytes[0], > lector_bytes[1])); > - dt_dive = NULL; > - return *dt_dive; > + return false; > } > > /* > @@ -649,7 +648,7 @@ static struct dive dt_dive_parser(FILE *archivo, struct > dive *dt_dive) > dt_dive->cylinder[0].end.mbar = dt_dive->cylinder[0].start.mbar > - > ((dt_dive->cylinder[0].gas_used.mliter / > dt_dive->cylinder[0].type.size.mliter) * 1000); > } > - return *dt_dive; > + return true; > } > > void datatrak_import(const char *file, struct dive_table *table) > @@ -670,11 +669,14 @@ void datatrak_import(const char *file, struct > dive_table *table) > *fileheader = read_file_header(archivo); > while (i < fileheader->divesNum) { > struct dive *ptdive = alloc_dive(); > - *ptdive = dt_dive_parser(archivo, ptdive); > - if (!ptdive) > + > + if (!dt_dive_parser(archivo, ptdive)) { > report_error(translate("gettextFromC", "Error: no > dive")); > + free(ptdive); > + } else { > + record_dive(ptdive); > + } > i++; > - record_dive(ptdive); > } > taglist_cleanup(&g_tag_list); > fclose(archivo); > -- > 2.1.4 > > _______________________________________________ > subsurface mailing list > [email protected] > http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface _______________________________________________ subsurface mailing list [email protected] http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
