[B.A.T.M.A.N.] [PATCHv2 1/2] alfred-gpsd: Calculate size of global buffer and not of a single char

[B.A.T.M.A.N.] [PATCHv2 1/2] alfred-gpsd: Calculate size of global buffer and not of a single char

[Sven Eckelmann]

The global buffer is 65536 bytes in size and not only 1 byte. This statement
evaluated to a negative value which was casted to an unsigned value (aka...
very large value).

This is a regression was introduced in 71dbc00fd879f1e592b07d8397f724fb3f69ac64
("alfred: Use strncpy instead of strcpy for string").

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
v2: Fixed prefix of the commit subject

 gpsd/alfred-gpsd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gpsd/alfred-gpsd.c b/gpsd/alfred-gpsd.c
index 84a0ded..d6cdfd6 100644
--- a/gpsd/alfred-gpsd.c
+++ b/gpsd/alfred-gpsd.c
@@ -301,7 +301,7 @@ static void gpsd_read_gpsd(struct globals *globals)
 	size_t cnt;
 	bool eol = false;
 	char buf[4096];
-	const size_t tpv_size = sizeof(*globals->buf) -
+	const size_t tpv_size = sizeof(globals->buf) -
 				sizeof(*globals->push) -
 				sizeof(struct alfred_data) -
 				sizeof(*globals->gpsd_data);
@@ -450,7 +450,7 @@ static int gpsd_server(struct globals *globals)
 	int max_fd, ret;
 	const size_t overhead = sizeof(*globals->push) +
 		sizeof(struct alfred_data);
-	const size_t tpv_size = sizeof(*globals->buf) -
+	const size_t tpv_size = sizeof(globals->buf) -
 				sizeof(*globals->push) -
 				sizeof(struct alfred_data) -
 				sizeof(*globals->gpsd_data);
-- 
2.0.0

[B.A.T.M.A.N.] [PATCHv2 2/2] alfred-gpsd: Avoid underrun when reading from gpsd

[Sven Eckelmann]

The gpsd output reading function is ignoring \r characters. This is done by
moving the current position (cnt) one position back in the character buffer. It
is jumping to the -1 character (max number for size_t) when it was reading the
first character at position 0. This is not problematic when the cnt is
increased directly after it by 1. Overflows/underflows are defined for
*unsigned* types and thus it just jumps back to 0.

Unfortunatelly, it is trying to access the memory for another check before
increasing the position again. This check is done on memory outside of the
buffer and therefore invalid.

Instead doing two check after each other, it is in this situation better to do
both at once and just handle the current character.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 gpsd/alfred-gpsd.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/gpsd/alfred-gpsd.c b/gpsd/alfred-gpsd.c
index d6cdfd6..87943bd 100644
--- a/gpsd/alfred-gpsd.c
+++ b/gpsd/alfred-gpsd.c
@@ -315,15 +315,16 @@ static void gpsd_read_gpsd(struct globals *globals)
 			return;
 		}
 
-		if (buf[cnt] == '\r')
+		switch (buf[cnt]) {
+		case '\r':
 			cnt--;
-
-		if (buf[cnt] == '\n') {
+			break;
+		case '\n':
 			eol = true;
 			buf[cnt] = '\0';
 			break;
 		}
-	} while (cnt++ < sizeof(buf) - 1);
+	} while (cnt++ < sizeof(buf) - 1 && !eol);
 
 	if (!eol) {
 		gps_close(&globals->gpsdata);
-- 
2.0.0

Re: [B.A.T.M.A.N.] [PATCHv2 1/2] alfred-gpsd: Calculate size of global buffer and not of a single char

[Simon Wunderlich]

> The global buffer is 65536 bytes in size and not only 1 byte. This
> statement evaluated to a negative value which was casted to an unsigned
> value (aka... very large value).
> 
> This is a regression was introduced in
> 71dbc00fd879f1e592b07d8397f724fb3f69ac64 ("alfred: Use strncpy instead of
> strcpy for string").

Both patches applied of this series (c67658a, 71b36fa).

Thanks!
    Simon

Re: [B.A.T.M.A.N.] [PATCHv2 2/2] alfred-gpsd: Avoid underrun when reading from gpsd

[Andrew Lunn]

On Wed, Jun 25, 2014 at 01:35:58PM +0200, Sven Eckelmann wrote:
> The gpsd output reading function is ignoring \r characters. This is done by
> moving the current position (cnt) one position back in the character buffer. It
> is jumping to the -1 character (max number for size_t) when it was reading the
> first character at position 0. This is not problematic when the cnt is
> increased directly after it by 1. Overflows/underflows are defined for
> *unsigned* types and thus it just jumps back to 0.
> 
> Unfortunatelly, it is trying to access the memory for another check before
> increasing the position again. This check is done on memory outside of the
> buffer and therefore invalid.
> 
> Instead doing two check after each other, it is in this situation better to do
> both at once and just handle the current character.
> 
> Signed-off-by: Sven Eckelmann <sven@narfation.org>

Hi Sven

Thanks for the patch. I've now tested it. Works fine.

Tested-by: Andrew Lunn <andrew@lunn.ch>

   Andrew


> ---
>  gpsd/alfred-gpsd.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/gpsd/alfred-gpsd.c b/gpsd/alfred-gpsd.c
> index d6cdfd6..87943bd 100644
> --- a/gpsd/alfred-gpsd.c
> +++ b/gpsd/alfred-gpsd.c
> @@ -315,15 +315,16 @@ static void gpsd_read_gpsd(struct globals *globals)
>  			return;
>  		}
>  
> -		if (buf[cnt] == '\r')
> +		switch (buf[cnt]) {
> +		case '\r':
>  			cnt--;
> -
> -		if (buf[cnt] == '\n') {
> +			break;
> +		case '\n':
>  			eol = true;
>  			buf[cnt] = '\0';
>  			break;
>  		}
> -	} while (cnt++ < sizeof(buf) - 1);
> +	} while (cnt++ < sizeof(buf) - 1 && !eol);
>  
>  	if (!eol) {
>  		gps_close(&globals->gpsdata);
> -- 
> 2.0.0
>