command: hashsum: md5sum reports wrong sum for the file named 4k.bin

command: hashsum: md5sum reports wrong sum for the file named 4k.bin

[Peter Mamonov]

Hi,

The md5sum command reports wrong sum for a file named 4k.bin and, I guess, for 
other files named alike. Here are the commands to reproduce the bug:

	barebox@barebox sandbox:/ memcpy -s /dev/zero -d _4k.bin 0 0 4k
	barebox@barebox sandbox:/ md5sum _4k.bin
	620f0b67a91f7f74151bc5be745b7110  _4k.bin
	
	barebox@barebox sandbox:/ cp _4k.bin 4k.bin
	barebox@barebox sandbox:/ md5sum 4k.bin
	d41d8cd98f00b204e9800998ecf8427e  4k.bin
	
	barebox@barebox sandbox:/ cp 4k.bin __4k.bin
	barebox@barebox sandbox:/ md5sum __4k.bin 
	620f0b67a91f7f74151bc5be745b7110  __4k.bin
	
	barebox@barebox sandbox:/ version
	
	barebox 2017.12.0 #4 Wed Dec 20 12:26:57 MSK 2017
	
	barebox@barebox sandbox:/ Terminated
	
	$ dd if=/dev/zero bs=4k count=1 | md5sum
	...
	620f0b67a91f7f74151bc5be745b7110  -

Regards,
Peter

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: command: hashsum: md5sum reports wrong sum for the file named 4k.bin

[Peter Mamonov]

On Wed, Dec 20, 2017 at 01:48:14PM +0300, Peter Mamonov wrote:
> Hi,
> 
> The md5sum command reports wrong sum for a file named 4k.bin and, I guess, for 
> other files named alike. Here are the commands to reproduce the bug:

The problem lays in __do_digest() function: 
 - it initializes variables `start` and `size` at commands/digest.c:38
 - then it calls parse_area_spec(*argv, &start, &size) at commands/digest.c:42
 - parse_area_spec() returns error as it should, however it changes the value 
 of the `start` at lib/misc.c:87.
 - this causes digest algo to skip leading 4k of the data from the file.

I'm not quite sure what is the best solution:
 - prevent parse_area_spec() from setting start if it fails?
 - reinitialize start/size after unsuccessfull call to parse_area_spec()?

Regards,
Peter


> 
> 	barebox@barebox sandbox:/ memcpy -s /dev/zero -d _4k.bin 0 0 4k
> 	barebox@barebox sandbox:/ md5sum _4k.bin
> 	620f0b67a91f7f74151bc5be745b7110  _4k.bin
> 	
> 	barebox@barebox sandbox:/ cp _4k.bin 4k.bin
> 	barebox@barebox sandbox:/ md5sum 4k.bin
> 	d41d8cd98f00b204e9800998ecf8427e  4k.bin
> 	
> 	barebox@barebox sandbox:/ cp 4k.bin __4k.bin
> 	barebox@barebox sandbox:/ md5sum __4k.bin 
> 	620f0b67a91f7f74151bc5be745b7110  __4k.bin
> 	
> 	barebox@barebox sandbox:/ version
> 	
> 	barebox 2017.12.0 #4 Wed Dec 20 12:26:57 MSK 2017
> 	
> 	barebox@barebox sandbox:/ Terminated
> 	
> 	$ dd if=/dev/zero bs=4k count=1 | md5sum
> 	...
> 	620f0b67a91f7f74151bc5be745b7110  -
> 
> Regards,
> Peter

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: command: hashsum: md5sum reports wrong sum for the file named 4k.bin

[Sascha Hauer]

Hi Peter,

On Thu, Dec 21, 2017 at 01:48:11PM +0300, Peter Mamonov wrote:
> On Wed, Dec 20, 2017 at 01:48:14PM +0300, Peter Mamonov wrote:
> > Hi,
> > 
> > The md5sum command reports wrong sum for a file named 4k.bin and, I guess, for 
> > other files named alike. Here are the commands to reproduce the bug:
> 
> The problem lays in __do_digest() function: 
>  - it initializes variables `start` and `size` at commands/digest.c:38
>  - then it calls parse_area_spec(*argv, &start, &size) at commands/digest.c:42
>  - parse_area_spec() returns error as it should, however it changes the value 
>  of the `start` at lib/misc.c:87.
>  - this causes digest algo to skip leading 4k of the data from the file.
> 
> I'm not quite sure what is the best solution:
>  - prevent parse_area_spec() from setting start if it fails?
>  - reinitialize start/size after unsuccessfull call to parse_area_spec()?

preventing parse_area_spec() from modifying start and size if it fails
seems like a good idea.

However, this doesn't solve the problem, does it? parse_area_spec()
only fails because the dot in 4k.bin is not valid, but if you name
the file only '4k' then parse_area_spec() won't fail.
The problem is that we cannot know whether '4k' is a file or an area.

Right now I have no good idea how to make the command nonambiguous,
but this will require an incompatible change to the commands arguments.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: command: hashsum: md5sum reports wrong sum for the file named 4k.bin

[Peter Mamonov]

On Fri, Jan 05, 2018 at 12:44:19PM +0100, Sascha Hauer wrote:
> Hi Peter,
> 
> On Thu, Dec 21, 2017 at 01:48:11PM +0300, Peter Mamonov wrote:
> > On Wed, Dec 20, 2017 at 01:48:14PM +0300, Peter Mamonov wrote:
> > > Hi,
> > > 
> > > The md5sum command reports wrong sum for a file named 4k.bin and, I guess, for 
> > > other files named alike. Here are the commands to reproduce the bug:
> > 
> > The problem lays in __do_digest() function: 
> >  - it initializes variables `start` and `size` at commands/digest.c:38
> >  - then it calls parse_area_spec(*argv, &start, &size) at commands/digest.c:42
> >  - parse_area_spec() returns error as it should, however it changes the value 
> >  of the `start` at lib/misc.c:87.
> >  - this causes digest algo to skip leading 4k of the data from the file.
> > 
> > I'm not quite sure what is the best solution:
> >  - prevent parse_area_spec() from setting start if it fails?
> >  - reinitialize start/size after unsuccessfull call to parse_area_spec()?
> 
> preventing parse_area_spec() from modifying start and size if it fails
> seems like a good idea.
> 
> However, this doesn't solve the problem, does it? parse_area_spec()
> only fails because the dot in 4k.bin is not valid, but if you name
> the file only '4k' then parse_area_spec() won't fail.
> The problem is that we cannot know whether '4k' is a file or an area.

You are right. However, there is no ambiguity in the "4k.bin" and similar 
cases, so we can fix at least this obvious problem.

Regards,
Peter

> 
> Right now I have no good idea how to make the command nonambiguous,
> but this will require an incompatible change to the commands arguments.
> 
> Sascha
> 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: command: hashsum: md5sum reports wrong sum for the file named 4k.bin

[Sascha Hauer]

On Fri, Jan 05, 2018 at 09:17:30PM +0300, Peter Mamonov wrote:
> On Fri, Jan 05, 2018 at 12:44:19PM +0100, Sascha Hauer wrote:
> > Hi Peter,
> > 
> > On Thu, Dec 21, 2017 at 01:48:11PM +0300, Peter Mamonov wrote:
> > > On Wed, Dec 20, 2017 at 01:48:14PM +0300, Peter Mamonov wrote:
> > > > Hi,
> > > > 
> > > > The md5sum command reports wrong sum for a file named 4k.bin and, I guess, for 
> > > > other files named alike. Here are the commands to reproduce the bug:
> > > 
> > > The problem lays in __do_digest() function: 
> > >  - it initializes variables `start` and `size` at commands/digest.c:38
> > >  - then it calls parse_area_spec(*argv, &start, &size) at commands/digest.c:42
> > >  - parse_area_spec() returns error as it should, however it changes the value 
> > >  of the `start` at lib/misc.c:87.
> > >  - this causes digest algo to skip leading 4k of the data from the file.
> > > 
> > > I'm not quite sure what is the best solution:
> > >  - prevent parse_area_spec() from setting start if it fails?
> > >  - reinitialize start/size after unsuccessfull call to parse_area_spec()?
> > 
> > preventing parse_area_spec() from modifying start and size if it fails
> > seems like a good idea.
> > 
> > However, this doesn't solve the problem, does it? parse_area_spec()
> > only fails because the dot in 4k.bin is not valid, but if you name
> > the file only '4k' then parse_area_spec() won't fail.
> > The problem is that we cannot know whether '4k' is a file or an area.
> 
> You are right. However, there is no ambiguity in the "4k.bin" and similar 
> cases, so we can fix at least this obvious problem.

Indeed, so a patch for parse_area_spec() would be welcomed.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH] lib: parse_area_spec: don't modify *start and *size values if parse failed

[Peter Mamonov]

Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
---
 lib/misc.c | 46 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 14 deletions(-)

diff --git a/lib/misc.c b/lib/misc.c
index 62ddd6677..c7d5a0ca5 100644
--- a/lib/misc.c
+++ b/lib/misc.c
@@ -79,38 +79,56 @@ EXPORT_SYMBOL(strtoul_suffix);
 int parse_area_spec(const char *str, loff_t *start, loff_t *size)
 {
 	char *endp;
-	loff_t end;
+	loff_t end, _start, _size;
+	int ret = -1;
 
 	if (!isdigit(*str))
 		return -1;
 
-	*start = strtoull_suffix(str, &endp, 0);
+	_start = strtoull_suffix(str, &endp, 0);
 
 	str = endp;
 
 	if (!*str) {
 		/* beginning given, but no size, assume maximum size */
-		*size = ~0;
-		return 0;
+		_size = ~0;
+		ret = 0;
 	}
 
-	if (*str == '-') {
+	if (ret && *str == '-') {
 		/* beginning and end given */
-		end = strtoull_suffix(str + 1, NULL, 0);
-		if (end < *start) {
+		if (!isdigit(*(str + 1)))
+			return ret;
+
+		end = strtoull_suffix(str + 1, &endp, 0);
+		str = endp;
+		if (end < _start) {
 			printf("end < start\n");
-			return -1;
+			return ret;
 		}
-		*size = end - *start + 1;
-		return 0;
+		_size = end - _start + 1;
+		ret = 0;
 	}
 
-	if (*str == '+') {
+	if (ret && *str == '+') {
 		/* beginning and size given */
-		*size = strtoull_suffix(str + 1, NULL, 0);
-		return 0;
+		if (!isdigit(*(str + 1)))
+			return ret;
+
+		_size = strtoull_suffix(str + 1, &endp, 0);
+		str = endp;
+		ret = 0;
+	}
+
+	if (!ret && *str)
+		/* trailing symbols indicate invalid area spec */
+		ret = -1;
+
+	if (!ret) {
+		*start = _start;
+		*size = _size;
 	}
 
-	return -1;
+	return ret;
 }
 EXPORT_SYMBOL(parse_area_spec);
-- 
2.11.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] lib: parse_area_spec: don't modify *start and *size values if parse failed

[Sascha Hauer]

Hi Peter,

On Tue, Jan 09, 2018 at 05:21:20PM +0300, Peter Mamonov wrote:
> Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
> ---
>  lib/misc.c | 46 ++++++++++++++++++++++++++++++++--------------
>  1 file changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/misc.c b/lib/misc.c
> index 62ddd6677..c7d5a0ca5 100644
> --- a/lib/misc.c
> +++ b/lib/misc.c
> @@ -79,38 +79,56 @@ EXPORT_SYMBOL(strtoul_suffix);
>  int parse_area_spec(const char *str, loff_t *start, loff_t *size)
>  {
>  	char *endp;
> -	loff_t end;
> +	loff_t end, _start, _size;
> +	int ret = -1;
>  
>  	if (!isdigit(*str))
>  		return -1;
>  
> -	*start = strtoull_suffix(str, &endp, 0);
> +	_start = strtoull_suffix(str, &endp, 0);
>  
>  	str = endp;
>  
>  	if (!*str) {
>  		/* beginning given, but no size, assume maximum size */
> -		*size = ~0;
> -		return 0;
> +		_size = ~0;
> +		ret = 0;
>  	}
>  
> -	if (*str == '-') {
> +	if (ret && *str == '-') {
>  		/* beginning and end given */
> -		end = strtoull_suffix(str + 1, NULL, 0);
> -		if (end < *start) {
> +		if (!isdigit(*(str + 1)))
> +			return ret;
> +
> +		end = strtoull_suffix(str + 1, &endp, 0);
> +		str = endp;
> +		if (end < _start) {
>  			printf("end < start\n");
> -			return -1;
> +			return ret;
>  		}
> -		*size = end - *start + 1;
> -		return 0;
> +		_size = end - _start + 1;
> +		ret = 0;
>  	}
>  
> -	if (*str == '+') {
> +	if (ret && *str == '+') {
>  		/* beginning and size given */
> -		*size = strtoull_suffix(str + 1, NULL, 0);
> -		return 0;
> +		if (!isdigit(*(str + 1)))
> +			return ret;
> +
> +		_size = strtoull_suffix(str + 1, &endp, 0);
> +		str = endp;
> +		ret = 0;
> +	}
> +
> +	if (!ret && *str)
> +		/* trailing symbols indicate invalid area spec */
> +		ret = -1;

Is this correct? I would assume a whitespace should be fine. We only
do not get trailing whitespaces in here because current users pass in
argv[] elements which are split up at whitespaces. The check would
also deserve a separate patch.

> +
> +	if (!ret) {
> +		*start = _start;
> +		*size = _size;
>  	}
>  
> -	return -1;
> +	return ret;

I find this patch unnecessarily hard to review and also the end result
doesn't look optimal. Could you create a 'success:' label and jump to it
when everything is fine? That would make the additional if(ret) and
if(!ret) checks unnecessary.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] lib: parse_area_spec: don't modify *start and *size values if parse failed

[Peter Mamonov]

Hi, Sasha,

On Thu, Jan 11, 2018 at 09:15:31AM +0100, Sascha Hauer wrote:
> Hi Peter,
> 
> On Tue, Jan 09, 2018 at 05:21:20PM +0300, Peter Mamonov wrote:
> > Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
> > ---
> >  lib/misc.c | 46 ++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 32 insertions(+), 14 deletions(-)
> > 
> > diff --git a/lib/misc.c b/lib/misc.c
> > index 62ddd6677..c7d5a0ca5 100644
> > --- a/lib/misc.c
> > +++ b/lib/misc.c
> > @@ -79,38 +79,56 @@ EXPORT_SYMBOL(strtoul_suffix);
> >  int parse_area_spec(const char *str, loff_t *start, loff_t *size)
> >  {
> >  	char *endp;
> > -	loff_t end;
> > +	loff_t end, _start, _size;
> > +	int ret = -1;
> >  
> >  	if (!isdigit(*str))
> >  		return -1;
> >  
> > -	*start = strtoull_suffix(str, &endp, 0);
> > +	_start = strtoull_suffix(str, &endp, 0);
> >  
> >  	str = endp;
> >  
> >  	if (!*str) {
> >  		/* beginning given, but no size, assume maximum size */
> > -		*size = ~0;
> > -		return 0;
> > +		_size = ~0;
> > +		ret = 0;
> >  	}
> >  
> > -	if (*str == '-') {
> > +	if (ret && *str == '-') {
> >  		/* beginning and end given */
> > -		end = strtoull_suffix(str + 1, NULL, 0);
> > -		if (end < *start) {
> > +		if (!isdigit(*(str + 1)))
> > +			return ret;
> > +
> > +		end = strtoull_suffix(str + 1, &endp, 0);
> > +		str = endp;
> > +		if (end < _start) {
> >  			printf("end < start\n");
> > -			return -1;
> > +			return ret;
> >  		}
> > -		*size = end - *start + 1;
> > -		return 0;
> > +		_size = end - _start + 1;
> > +		ret = 0;
> >  	}
> >  
> > -	if (*str == '+') {
> > +	if (ret && *str == '+') {
> >  		/* beginning and size given */
> > -		*size = strtoull_suffix(str + 1, NULL, 0);
> > -		return 0;
> > +		if (!isdigit(*(str + 1)))
> > +			return ret;
> > +
> > +		_size = strtoull_suffix(str + 1, &endp, 0);
> > +		str = endp;
> > +		ret = 0;
> > +	}
> > +
> > +	if (!ret && *str)
> > +		/* trailing symbols indicate invalid area spec */
> > +		ret = -1;
> 
> Is this correct? I would assume a whitespace should be fine. We only
> do not get trailing whitespaces in here because current users pass in
> argv[] elements which are split up at whitespaces.

Ok, whitespaces " \n\r\t" are fine too. Will fix it in the next revision.

> The check would
> also deserve a separate patch.

My proposal is to fix `parse_area_spec` so it can distinguish a valid memory 
area specification from the following sample file names:
	4k.bin
	4k-8k.txt
	4096+1k_of_random_bytes

Without this final check parse_area_spec would return 0 for the last two 
samples, which are not valid area specifications.

> 
> > +
> > +	if (!ret) {
> > +		*start = _start;
> > +		*size = _size;
> >  	}
> >  
> > -	return -1;
> > +	return ret;
> 
> I find this patch unnecessarily hard to review and also the end result
> doesn't look optimal. Could you create a 'success:' label and jump to it
> when everything is fine? That would make the additional if(ret) and
> if(!ret) checks unnecessary.

Sounds good, will fix it as well.

Regards,
Peter

> 
> Sascha
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 1/3] lib: parse_area_spec: don't modify *start value if parse failed

[Peter Mamonov]

Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
---
 lib/misc.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/lib/misc.c b/lib/misc.c
index 62ddd6677..1767043d1 100644
--- a/lib/misc.c
+++ b/lib/misc.c
@@ -79,19 +79,19 @@ EXPORT_SYMBOL(strtoul_suffix);
 int parse_area_spec(const char *str, loff_t *start, loff_t *size)
 {
 	char *endp;
-	loff_t end;
+	loff_t end, _start;
 
 	if (!isdigit(*str))
 		return -1;
 
-	*start = strtoull_suffix(str, &endp, 0);
+	_start = strtoull_suffix(str, &endp, 0);
 
 	str = endp;
 
 	if (!*str) {
 		/* beginning given, but no size, assume maximum size */
 		*size = ~0;
-		return 0;
+		goto success;
 	}
 
 	if (*str == '-') {
@@ -102,15 +102,19 @@ int parse_area_spec(const char *str, loff_t *start, loff_t *size)
 			return -1;
 		}
 		*size = end - *start + 1;
-		return 0;
+		goto success;
 	}
 
 	if (*str == '+') {
 		/* beginning and size given */
 		*size = strtoull_suffix(str + 1, NULL, 0);
-		return 0;
+		goto success;
 	}
 
 	return -1;
+
+success:
+	*start = _start;
+	return 0;
 }
 EXPORT_SYMBOL(parse_area_spec);
-- 
2.11.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 2/3] lib: parse_area_spec: part of the area spec after -/+ should start with a digit

[Peter Mamonov]

Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
---
 lib/misc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/misc.c b/lib/misc.c
index 1767043d1..fc2c45aa4 100644
--- a/lib/misc.c
+++ b/lib/misc.c
@@ -96,6 +96,9 @@ int parse_area_spec(const char *str, loff_t *start, loff_t *size)
 
 	if (*str == '-') {
 		/* beginning and end given */
+		if (!isdigit(*(str + 1)))
+			return -1;
+
 		end = strtoull_suffix(str + 1, NULL, 0);
 		if (end < *start) {
 			printf("end < start\n");
@@ -107,6 +110,9 @@ int parse_area_spec(const char *str, loff_t *start, loff_t *size)
 
 	if (*str == '+') {
 		/* beginning and size given */
+		if (!isdigit(*(str + 1)))
+			return -1;
+
 		*size = strtoull_suffix(str + 1, NULL, 0);
 		goto success;
 	}
-- 
2.11.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 3/3] lib: parse_area_spec: no extra symbols after area spec are allowed except for spaces

[Peter Mamonov]

Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
---
 lib/misc.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/lib/misc.c b/lib/misc.c
index fc2c45aa4..4b62f8771 100644
--- a/lib/misc.c
+++ b/lib/misc.c
@@ -79,7 +79,7 @@ EXPORT_SYMBOL(strtoul_suffix);
 int parse_area_spec(const char *str, loff_t *start, loff_t *size)
 {
 	char *endp;
-	loff_t end, _start;
+	loff_t end, _start, _size;
 
 	if (!isdigit(*str))
 		return -1;
@@ -90,7 +90,7 @@ int parse_area_spec(const char *str, loff_t *start, loff_t *size)
 
 	if (!*str) {
 		/* beginning given, but no size, assume maximum size */
-		*size = ~0;
+		_size = ~0;
 		goto success;
 	}
 
@@ -99,12 +99,13 @@ int parse_area_spec(const char *str, loff_t *start, loff_t *size)
 		if (!isdigit(*(str + 1)))
 			return -1;
 
-		end = strtoull_suffix(str + 1, NULL, 0);
+		end = strtoull_suffix(str + 1, &endp, 0);
+		str = endp;
 		if (end < *start) {
 			printf("end < start\n");
 			return -1;
 		}
-		*size = end - *start + 1;
+		_size = end - *start + 1;
 		goto success;
 	}
 
@@ -113,14 +114,18 @@ int parse_area_spec(const char *str, loff_t *start, loff_t *size)
 		if (!isdigit(*(str + 1)))
 			return -1;
 
-		*size = strtoull_suffix(str + 1, NULL, 0);
+		_size = strtoull_suffix(str + 1, &endp, 0);
+		str = endp;
 		goto success;
 	}
 
 	return -1;
 
 success:
+	if (*str && !isspace(*str))
+		return -1;
 	*start = _start;
+	*size = _size;
 	return 0;
 }
 EXPORT_SYMBOL(parse_area_spec);
-- 
2.11.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 1/3] lib: parse_area_spec: don't modify *start value if parse failed

[Sascha Hauer]

On Mon, Jan 15, 2018 at 02:32:31PM +0300, Peter Mamonov wrote:
> Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
> ---
>  lib/misc.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/misc.c b/lib/misc.c
> index 62ddd6677..1767043d1 100644
> --- a/lib/misc.c
> +++ b/lib/misc.c
> @@ -79,19 +79,19 @@ EXPORT_SYMBOL(strtoul_suffix);
>  int parse_area_spec(const char *str, loff_t *start, loff_t *size)
>  {
>  	char *endp;
> -	loff_t end;
> +	loff_t end, _start;
>  
>  	if (!isdigit(*str))
>  		return -1;
>  
> -	*start = strtoull_suffix(str, &endp, 0);
> +	_start = strtoull_suffix(str, &endp, 0);
>  
>  	str = endp;
>  
>  	if (!*str) {
>  		/* beginning given, but no size, assume maximum size */
>  		*size = ~0;
> -		return 0;
> +		goto success;
>  	}
>  
>  	if (*str == '-') {
> @@ -102,15 +102,19 @@ int parse_area_spec(const char *str, loff_t *start, loff_t *size)
>  			return -1;
>  		}
>  		*size = end - *start + 1;

Applied with two little changes: It must be _start above and in another
case not visible in the patch. *start is not yet initialized.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 1/3] lib: parse_area_spec: don't modify *start value if parse failed

[Peter Mamonov]

On Wed, Jan 17, 2018 at 09:11:12AM +0100, Sascha Hauer wrote:
> On Mon, Jan 15, 2018 at 02:32:31PM +0300, Peter Mamonov wrote:
> > Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
> > ---
> >  lib/misc.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/lib/misc.c b/lib/misc.c
> > index 62ddd6677..1767043d1 100644
> > --- a/lib/misc.c
> > +++ b/lib/misc.c
> > @@ -79,19 +79,19 @@ EXPORT_SYMBOL(strtoul_suffix);
> >  int parse_area_spec(const char *str, loff_t *start, loff_t *size)
> >  {
> >  	char *endp;
> > -	loff_t end;
> > +	loff_t end, _start;
> >  
> >  	if (!isdigit(*str))
> >  		return -1;
> >  
> > -	*start = strtoull_suffix(str, &endp, 0);
> > +	_start = strtoull_suffix(str, &endp, 0);
> >  
> >  	str = endp;
> >  
> >  	if (!*str) {
> >  		/* beginning given, but no size, assume maximum size */
> >  		*size = ~0;
> > -		return 0;
> > +		goto success;
> >  	}
> >  
> >  	if (*str == '-') {
> > @@ -102,15 +102,19 @@ int parse_area_spec(const char *str, loff_t *start, loff_t *size)
> >  			return -1;
> >  		}
> >  		*size = end - *start + 1;
> 
> Applied with two little changes: It must be _start above and in another
> case not visible in the patch. *start is not yet initialized.

Thanks for correction. Actually these changes were present in the original
patch, but I've missed them while rearranging it.

Regards,
Peter

> 
> Sascha
> 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox