[RFC PATCH] Datatrak parsing

[Dirk Hohndel]

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 <salvador.cunat at gmail.com>

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 <miika.turkia at gmail.com>
> 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 <miika.turkia at gmail.com>
> ---
> 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
> subsurface at subsurface-divelog.org
> http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface