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

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

Quoted 4 lines · 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:

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()?

Hi Peter,

Quoted 13 lines · Peter Mamonov

Quoted 4 lines · 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:

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.

Quoted 3 lines

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

Signature · 4 lines

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 |

Quoted 23 lines · Sascha Hauer

Hi Peter,

Quoted 13 lines · Peter Mamonov

Quoted 4 lines · 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:

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.

Quoted 3 lines

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.

Quoted 4 lines

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.

Signed-off-by Peter Mamonov <[email protected]>

---
 lib/misc.c | 46 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 14 deletions(-)

lib/misc.c +17 -13

index 62ddd6677..c7d5a0ca5 100644
@@ -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)))

Hi Peter,

Quoted 66 lines · Peter Mamonov

Signed-off-by Peter Mamonov <[email protected]>

---
 lib/misc.c | 46 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 14 deletions(-)

lib/misc.c +23 -13

index 62ddd6677..c7d5a0ca5 100644
@@ -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;
+	}

lib/misc.c EXPORT_SYMBOL(strtoul_suffix); · line 125

Quoted 4 lines

lib/misc.c +4 -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.

Quoted 7 lines

lib/misc.c +4 -0

+
+	if (!ret) {
+		*start = _start;
+		*size = _size;
 	}
 

lib/misc.c EXPORT_SYMBOL(strtoul_suffix); · line 132

Quoted 2 lines

lib/misc.c +1 -1

-	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

Signature · 4 lines

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 |

Signed-off-by Peter Mamonov <[email protected]>

---
 lib/misc.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

lib/misc.c +3 -3

index 62ddd6677..1767043d1 100644
@@ -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 == '-') {

Quoted 31 lines · Peter Mamonov

Signed-off-by Peter Mamonov <[email protected]>

---
 lib/misc.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

lib/misc.c +3 -3

index 62ddd6677..1767043d1 100644
@@ -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;
 	}
 

lib/misc.c int parse_area_spec(const char *str, loff_t *start, loff_t *size) · line 104

Quoted 5 lines

lib/misc.c +0 -0

 	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

Signature · 4 lines

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 |