bug#24874: dd: misleading parsing of hex numbers

[Jim Meyering]

On Fri, Nov 4, 2016 at 12:03 PM, Pádraig Brady  wrote:
> On 04/11/16 16:20, Pádraig Brady wrote:
>> On 04/11/16 11:19, Stephan Bauroth wrote:
>>> Dear coreutils team :)
>>>
>>> I encountered a buglike behaviour of dd when handling skip and count
>>> parameters that are encoded in hex and thus prefixed with 0x.
>>>
>>> dd is not able to parse them, which is OK but would be great if it would
>>> be, but, worse, reads 0xf00 as 0. It does that silently. While an
>>> enduser will immediately notice this on count, since nothing is copied,
>>> behaviour for skip looks ok. (In fact, I noticed this only because I
>>> hexdumped the result after hours of debugging)
>>>
>>> While it's OK that dd can't parse these numbers, maybe there should be a
>>> warning that 0x was found and interpreted as 0. Since a char like 'x' is
>>> invalid within a number that by definition has to be decimal, a warning
>>> should be fairly easy to implement.
>>>
>>> Of course, the ability to parse hex numbers in these parameters would be
>>> awesome :)
>>>
>>> regards and thanks for your continuing work,
>>> Stephan Bauroth
>>
>> Ouch. That's a real gotcha.
>> Note hex digits after the 0x are diagnosed, but not decimal digits:
>>
>>   $ dd skip=0x100 seek=0xf00
>>   dd: invalid number: ‘0xf00’
>>
>> Disallowing 0x... could definitely break backwards compat though.
>> Consider `for rec in 0 1 2; do dd skip=${rec}x1024...`
>>
>> I suppose we could output a warning to suggest using
>>   $(($rec * $size)) or 0${rec}x${size}
>> if that really is the intention?
>>
>> Given the warning workaround would be suggested in the message,
>> and that it's a relatively rare usage, a warning is probably appropriate 
>> here.
>> We already warn in dd for various usage.
>>
>> I'll fix that for the coming release.
>
> Patch attached.

Ouch, confusing indeed. I like your solution. One nit:

- dd if=/dev/null count=0x1 seek=0x1 skip=0x1 status=none 2>err
+ dd if=/dev/null count=0x1 seek=0x1 skip=0x1 status=none 2>err || fail=1

bug#24874: dd: misleading parsing of hex numbers

[Pádraig Brady]

On 04/11/16 16:20, Pádraig Brady wrote:
> On 04/11/16 11:19, Stephan Bauroth wrote:
>> Dear coreutils team :)
>>
>> I encountered a buglike behaviour of dd when handling skip and count 
>> parameters that are encoded in hex and thus prefixed with 0x.
>>
>> dd is not able to parse them, which is OK but would be great if it would 
>> be, but, worse, reads 0xf00 as 0. It does that silently. While an 
>> enduser will immediately notice this on count, since nothing is copied, 
>> behaviour for skip looks ok. (In fact, I noticed this only because I 
>> hexdumped the result after hours of debugging)
>>
>> While it's OK that dd can't parse these numbers, maybe there should be a 
>> warning that 0x was found and interpreted as 0. Since a char like 'x' is 
>> invalid within a number that by definition has to be decimal, a warning 
>> should be fairly easy to implement.
>>
>> Of course, the ability to parse hex numbers in these parameters would be 
>> awesome :)
>>
>> regards and thanks for your continuing work,
>> Stephan Bauroth
> 
> Ouch. That's a real gotcha.
> Note hex digits after the 0x are diagnosed, but not decimal digits:
> 
>   $ dd skip=0x100 seek=0xf00
>   dd: invalid number: ‘0xf00’
> 
> Disallowing 0x... could definitely break backwards compat though.
> Consider `for rec in 0 1 2; do dd skip=${rec}x1024...`
> 
> I suppose we could output a warning to suggest using
>   $(($rec * $size)) or 0${rec}x${size}
> if that really is the intention?
> 
> Given the warning workaround would be suggested in the message,
> and that it's a relatively rare usage, a warning is probably appropriate here.
> We already warn in dd for various usage.
> 
> I'll fix that for the coming release.

Patch attached.

>From 490edf8df09348eb2304392415f91ba4319840b5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= 
Date: Fri, 4 Nov 2016 16:55:58 +
Subject: [PATCH] dd: warn about counts specified with confusing 0x prefix

* src/dd.c (parse_integer): Suggest to use "00x" instead of "0x",
which is significant for the "count", "seek", and "skip" operands.
* tests/dd/misc.sh: Add a test case.
Fixes http://bugs.gnu.org/24874
---
 src/dd.c |  6 ++
 tests/dd/misc.sh | 10 ++
 2 files changed, 16 insertions(+)

diff --git a/src/dd.c b/src/dd.c
index 2c6d4c6..c7f54d4 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -1342,6 +1342,12 @@ parse_integer (const char *str, strtol_error *invalid)
   return 0;
 }
 
+  if (n == 0 && STRPREFIX (str, "0x"))
+error (0, 0,
+   _("warning: %s is a zero multiplier; "
+ "use %s if that is intended"),
+   quote_n (0, "0x"), quote_n (1, "00x"));
+
   n *= multiplier;
 }
   else if (e != LONGINT_OK)
diff --git a/tests/dd/misc.sh b/tests/dd/misc.sh
index 2c24b50..41330d0 100755
--- a/tests/dd/misc.sh
+++ b/tests/dd/misc.sh
@@ -107,4 +107,14 @@ compare err_ok err || fail=1
 
 test $fail -eq 0 && fail=$warn
 
+# Check a warning is issued for ambiguous 0x... numbers
+dd if=/dev/null count=0x1 seek=0x1 skip=0x1 status=none 2>err
+cat <<\EOF >exp
+dd: warning: '0x' is a zero multiplier; use '00x' if that is intended
+dd: warning: '0x' is a zero multiplier; use '00x' if that is intended
+dd: warning: '0x' is a zero multiplier; use '00x' if that is intended
+EOF
+compare exp err || fail=1
+
+
 Exit $fail
-- 
2.5.5

bug#24874: dd: misleading parsing of hex numbers

[Pádraig Brady]

On 04/11/16 11:19, Stephan Bauroth wrote:
> Dear coreutils team :)
> 
> I encountered a buglike behaviour of dd when handling skip and count 
> parameters that are encoded in hex and thus prefixed with 0x.
> 
> dd is not able to parse them, which is OK but would be great if it would 
> be, but, worse, reads 0xf00 as 0. It does that silently. While an 
> enduser will immediately notice this on count, since nothing is copied, 
> behaviour for skip looks ok. (In fact, I noticed this only because I 
> hexdumped the result after hours of debugging)
> 
> While it's OK that dd can't parse these numbers, maybe there should be a 
> warning that 0x was found and interpreted as 0. Since a char like 'x' is 
> invalid within a number that by definition has to be decimal, a warning 
> should be fairly easy to implement.
> 
> Of course, the ability to parse hex numbers in these parameters would be 
> awesome :)
> 
> regards and thanks for your continuing work,
> Stephan Bauroth

Ouch. That's a real gotcha.
Note hex digits after the 0x are diagnosed, but not decimal digits:

  $ dd skip=0x100 seek=0xf00
  dd: invalid number: ‘0xf00’

Disallowing 0x... could definitely break backwards compat though.
Consider `for rec in 0 1 2; do dd skip=${rec}x1024...`

I suppose we could output a warning to suggest using
  $(($rec * $size)) or 0${rec}x${size}
if that really is the intention?

Given the warning workaround would be suggested in the message,
and that it's a relatively rare usage, a warning is probably appropriate here.
We already warn in dd for various usage.

I'll fix that for the coming release.

thanks,
Pádraig

bug#24874: dd: misleading parsing of hex numbers

[Stephan Bauroth]

Dear coreutils team :)

I encountered a buglike behaviour of dd when handling skip and count 
parameters that are encoded in hex and thus prefixed with 0x.


dd is not able to parse them, which is OK but would be great if it would 
be, but, worse, reads 0xf00 as 0. It does that silently. While an 
enduser will immediately notice this on count, since nothing is copied, 
behaviour for skip looks ok. (In fact, I noticed this only because I 
hexdumped the result after hours of debugging)


While it's OK that dd can't parse these numbers, maybe there should be a 
warning that 0x was found and interpreted as 0. Since a char like 'x' is 
invalid within a number that by definition has to be decimal, a warning 
should be fairly easy to implement.


Of course, the ability to parse hex numbers in these parameters would be 
awesome :)


regards and thanks for your continuing work,
Stephan Bauroth