Bug 191263
| Summary: | [patch] dd(1): Incorrect casting of arguments | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | will | ||||||||||||||
| Component: | bin | Assignee: | Alan Somers <asomers> | ||||||||||||||
| Status: | Closed FIXED | ||||||||||||||||
| Severity: | Affects Many People | CC: | asomers, pi | ||||||||||||||
| Priority: | --- | Flags: | asomers:
mfc-stable11?
asomers: mfc-stable10? |
||||||||||||||
| Version: | CURRENT | ||||||||||||||||
| Hardware: | Any | ||||||||||||||||
| OS: | Any | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
Created attachment 147236 [details]
Better argument handling patch
Created attachment 147516 [details]
Better argument handling patch
(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 ? 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. Created attachment 147543 [details]
Better argument handling patch with fix for count=SIZE_MAX
(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 ? Created attachment 148683 [details]
Fix for dd's argument processing
Apparently the problem was that I didn't submit the latest patch. :/
Thanks. This behaves as described. I'll try to find someone to commit it. 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 Committed, thanks for you patience. TODO: merge into older versions ? 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. Unfortunately I do not have arm test hosts readily available to analyze. I can fix the format errors and resubmit though. 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 (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) 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.
(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 ? In this case, because the size of intmax_t is the same on 32bit arm and amd64, the test cases are the same. (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-) Close PRs that have had a corresponding fix committed. asomers suggests to reopen this PR. 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 |
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.