Bug 191263 - [patch] dd(1): Incorrect casting of arguments
Summary: [patch] dd(1): Incorrect casting of arguments
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: Alan Somers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-22 08:13 UTC by will
Modified: 2018-05-28 19:38 UTC (History)
2 users (show)

See Also:
asomers: mfc-stable11?
asomers: mfc-stable10?


Attachments
Better argument handling patch (2.65 KB, patch)
2014-06-22 08:13 UTC, will
no flags Details | Diff
Better argument handling patch (7.97 KB, text/plain)
2014-09-12 05:07 UTC, will
no flags Details
Better argument handling patch (6.81 KB, patch)
2014-09-20 23:19 UTC, will
no flags Details | Diff
Better argument handling patch with fix for count=SIZE_MAX (6.81 KB, patch)
2014-09-21 21:45 UTC, will
no flags Details | Diff
Fix for dd's argument processing (6.77 KB, patch)
2014-10-27 01:27 UTC, will
no flags Details | Diff
Processes arguments correctly (5.30 KB, patch)
2014-10-29 05:38 UTC, will
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description will 2014-06-22 08:13:03 UTC
Created attachment 144014 [details]
Better argument handling patch

dd(1) casts many of its numeric arguments from uintmax_t to intmax_t and back again to detect whether or not the original arguments were negative. This is not correct.

Attached is a patch that removes the pointless casts, and detects negative numbers slightly more reliably.
Comment 1 will 2014-09-12 05:07:28 UTC
Created attachment 147236 [details]
Better argument handling patch
Comment 2 will 2014-09-20 23:19:13 UTC
Created attachment 147516 [details]
Better argument handling patch
Comment 3 Kurt Jaeger freebsd_committer 2014-09-21 06:59:59 UTC
(In reply to will from comment #2)
> Created attachment 147516 [details]
> Better argument handling patch

Thanks, patch applies and builds now.

Do you have a test case which demonstrates the old error case that can be
used to test whether the new version is fixed ?
Comment 4 will 2014-09-21 21:44:38 UTC
Old:
 [ worr on terra ] ( ~ ) % dd if=/dev/zero of=/dev/null count=18446744073709551616
dd: count: Result too large
 [ worr on terra ] ( ~ ) % dd if=/dev/zero of=/dev/null count=18446744073709551615
dd: count cannot be negative
 [ worr on terra ] ( ~ ) % dd if=/dev/zero of=/dev/null count=18446744073709551614
dd: count cannot be negative

New:
 [ root on dev ] ( ~ ) # ./base/bin/dd/dd if=/dev/zero of=/dev/null count=18446744073709551616
dd: count: Result too large
 [ root on dev ] ( ~ ) # ./base/bin/dd/dd if=/dev/zero of=/dev/null count=18446744073709551615
dd: count: Result too large
 [ root on dev ] ( ~ ) # ./base/bin/dd/dd if=/dev/zero of=/dev/null count=18446744073709551614
^C1997704+0 records in
1997704+0 records out
1022824448 bytes transferred in 1.030049 secs (992986238 bytes/sec)

My last patch had a small error in it where it wouldn't accept SIZE_MAX as an argument to count (special handling happens here for the case where count is unspecified). What should be the final patch is now posted.
Comment 5 will 2014-09-21 21:45:48 UTC
Created attachment 147543 [details]
Better argument handling patch with fix for count=SIZE_MAX
Comment 6 Kurt Jaeger freebsd_committer 2014-10-21 18:42:40 UTC
(In reply to will from comment #5)
> Created attachment 147543 [details]
> Better argument handling patch with fix for count=SIZE_MAX

Thanks. Patch build-tested against src/bin/dd/ from HEAD, builds fine on
10.0p9-amd64.

You have 3 test cases, with ...16, ...15 and ...14 as count values for dd.

Interestingly the ...15 case behaves different from your example ?

$ ./dd if=/dev/zero of=/dev/null count=18446744073709551615
0+0 records in
0+0 records out
0 bytes transferred in 0.000007 secs (0 bytes/sec)

What's the difference ?
Comment 7 will 2014-10-27 01:27:44 UTC
Created attachment 148683 [details]
Fix for dd's argument processing

Apparently the problem was that I didn't submit the latest patch. :/
Comment 8 Kurt Jaeger freebsd_committer 2014-10-27 06:28:27 UTC
Thanks. This behaves as described. I'll try to find someone to commit it.
Comment 9 commit-hook freebsd_committer 2014-10-27 11:38:27 UTC
A commit references this bug:

Author: pi
Date: Mon Oct 27 11:38:19 UTC 2014
New revision: 273734
URL: https://svnweb.freebsd.org/changeset/base/273734

Log:
  bin/dd: Fix incorrect casting of arguments

  dd(1) casts many of its numeric arguments from uintmax_t to intmax_t
  and back again to detect whether or not the original arguments were
  negative. This caused wrong behaviour in some boundary cases:

  $ dd if=/dev/zero of=/dev/null count=18446744073709551615
  dd: count cannot be negative

  After the fix:

  $ dd if=/dev/zero of=/dev/null count=18446744073709551615
  dd: count: Result too large

  PR:		191263
  Submitted by:	will@worrbase.com
  Approved by:	cognet@

Changes:
  head/bin/dd/args.c
  head/bin/dd/conv.c
  head/bin/dd/dd.c
  head/bin/dd/dd.h
  head/bin/dd/position.c
Comment 10 Kurt Jaeger freebsd_committer 2014-10-27 11:39:14 UTC
Committed, thanks for you patience.
Comment 11 Kurt Jaeger freebsd_committer 2014-10-27 11:41:00 UTC
TODO: merge into older versions ?
Comment 12 Kurt Jaeger freebsd_committer 2014-10-27 15:57:24 UTC
Konstantin Belousov reports problems on arm, see:

https://lists.freebsd.org/pipermail/svn-src-all/2014-October/093782.html

cc1: warnings being treated as errors
/scratch/tmp/kib/src/bin/dd/args.c: In function 'f_bs':
/scratch/tmp/kib/src/bin/dd/args.c:192: warning: format '%jd' expects type 'intm
ax_t', but argument 3 has type 'int'

Do you have a chance to analyse it on arm ? I'll try to find a way for
a test host.
Comment 13 will 2014-10-27 16:28:28 UTC
Unfortunately I do not have arm test hosts readily available to analyze. I can fix the format errors and resubmit though.
Comment 14 commit-hook freebsd_committer 2014-10-27 17:40:02 UTC
A commit references this bug:

Author: pi
Date: Mon Oct 27 17:39:38 UTC 2014
New revision: 273743
URL: https://svnweb.freebsd.org/changeset/base/273743

Log:
  bin/dd: revert 273734, as it fails on 32bit platforms

  Revert: insufficient testing on 32bit platforms

  PR:		191263

Changes:
  head/bin/dd/args.c
  head/bin/dd/conv.c
  head/bin/dd/dd.c
  head/bin/dd/dd.h
  head/bin/dd/position.c
Comment 15 Kurt Jaeger freebsd_committer 2014-10-27 19:19:45 UTC
(In reply to Kurt Jaeger from comment #12)
> Do you have a chance to analyse it on arm ? I'll try to find a way for
> a test host.

I found a 10.1-RC3-i386 test host, build the code and tested your
test cases:

$ ./dd if=/dev/zero of=/dev/null count=18446744073709551616
dd: count: Result too large
$ ./dd if=/dev/zero of=/dev/null count=18446744073709551615
0+0 records in
2883718591385954792+4546498139456461995 records out
3334595201111949312 bytes transferred in 0.000000 secs (0 bytes/sec)
$ ./dd if=/dev/zero of=/dev/null count=18446744073709551614
^?134403+134403 records in
577736087679054362+4607448192116238825 records out
3178039305324594688 bytes transferred in 567918259069603272652283816685764345856.000000 secs (0 bytes/sec)
Comment 16 will 2014-10-29 05:38:15 UTC
Created attachment 148754 [details]
Processes arguments correctly

After testing on 32bit arm and amd64, I've fixed my patch.

The original patch made many incorrect assumptions about the size of size_t that only held true on amd64. This patch does not make such assumptions. It's much simpler than any of the patches put forth before.
Comment 17 Kurt Jaeger freebsd_committer 2014-10-29 06:43:19 UTC
(In reply to will from comment #16)
> Created attachment 148754 [details]
> Processes arguments correctly
> 
> After testing on 32bit arm and amd64, I've fixed my patch.

Do you have testcases for the 32bit boundary cases ?
Comment 18 will 2014-10-29 17:11:41 UTC
In this case, because the size of intmax_t is the same on 32bit arm and amd64, the test cases are the same.
Comment 19 Kurt Jaeger freebsd_committer 2014-10-29 20:04:24 UTC
(In reply to will from comment #18)
> In this case, because the size of intmax_t is the same on 32bit arm and
> amd64, the test cases are the same.

Thanks. I tested it on arm and i386 as well, builds fine and tests
work as your example says that they should work.

I'll try to find some committer for it, before I get burned 8-)
Comment 20 Glen Barber freebsd_committer 2015-07-08 18:14:48 UTC
Close PRs that have had a corresponding fix committed.
Comment 21 Kurt Jaeger freebsd_committer 2017-06-05 15:44:44 UTC
asomers suggests to reopen this PR.
Comment 22 commit-hook freebsd_committer 2017-08-25 15:32:57 UTC
A commit references this bug:

Author: asomers
Date: Fri Aug 25 15:31:56 UTC 2017
New revision: 322893
URL: https://svnweb.freebsd.org/changeset/base/322893

Log:
  dd(1): Incorrect casting of arguments

  dd(1) casts many of its numeric arguments from uintmax_t to intmax_t and
  back again to detect whether or not the original arguments were negative.
  This is not correct, and causes problems with boundary cases, for example
  when count is SSIZE_MAX-1.

  PR:		191263
  Submitted by:	will@worrbase.com
  Reviewed by:	pi, asomers
  MFC after:	3 weeks

Changes:
  head/bin/dd/args.c
  head/bin/dd/conv.c
  head/bin/dd/dd.c
  head/bin/dd/dd.h
  head/bin/dd/position.c