|Subject:||[bug-diffutils] bug#35256: Bug report for -W argument (maximum width) - minor and not dangerous|
|Date:||Sat, 13 Apr 2019 12:29:45 +0100|
I was hoping to view a side-by-side diff of something and, perhaps unfairly, was hoping for a setting where diff would choose a width such that there were no truncations and I would use less with no wrapping to inspect the results.
My first attempt was "-W 0" (a width of 0 has no "legit" meaning afterall) - error, so I tried -1. This leads to a weird situation where it seems to just output loads of tabs - while it'll probably still terminate eventually the behaviour is unreasonable.
To try this yourself run something like:
diff -y ./maps ./task/4974/maps -W -1
from /proc/XXXX where XXXX is some PID for a program with threads (eg firefox) and the 4974 is any task that isn't XXXX
ADDENDUM: The -1 isn't important, 9999999999999 also illustrates the problem - END ADDENDUM
Looking at the code (in the 3.7 tarball, src/diff.c modified on 18th of December 2018) notice:
numval = strtoumax (optarg, &numend, 10);
if (! (0 < numval && numval <= SIZE_MAX) || *numend)
try_help ("invalid width '%s'", optarg);
if (width != numval)
fatal ("conflicting width options");
width = numval;
uintmax_t strtoumax(const char *nptr, char **endptr, int base);
and it may set errno, my man page doesn't say whether this -1 behaviour is "okay" however it probably is, unsigned afterall, this means that numval is going to be a really really big value.
Just basic DOS (denial-of-service) stuff, a CPU usage spike comes from diff itself and it seems to output a lot of tabs (a good 275mib / sec on my machine) and will probably do so for a good few years before anything else comes out, a testament to the robustness of diff is that it did this, and its memory usage didn't start ballooning.
I know diff is used by A LOT of other programs, some of which are web-accessible (eg mediawiki uses diff - and will by default if it finds it), many of my projects use it too. It is not a big stretch to imagine someone has a web-service out there which allows side-by-side format, and not much of a further leap to assume that someone might have an input box for width which exposes -W, guarded only by a regex of the form ^[1-9][0-9]* (which yes, wont allow -1 but will allow 9999999999999)
You could bring a server to its knees pretty quickly using just diff's CPU usage and a few tabs using this - that's not even considering whether or not the system hypothesised here doesn't have trouble with memory from a convenient get_line() function first.
While not really diff's fault or problem, a potential solution detailed below would fix it and not cause any problems for those with legit (?) needs for really wide diffs
Humans are limiting here, improvements and the growth of computers wont really affect the maximum width so putting a limit in place is reasonable. I make no claim there is a "maximum useful width" so being able to override will ensure my half-assed musings on such a limit wont cause any problems in the future.
I'd go with something like
#define REASONABLE_LIMIT 1000
Add a check that numval is <= get_reasonable_specified_width_limit() after the existing checks, if not output an error in the form of:
"You probably don't want to do that, see [wherever], if you do specify --we-have-evolved-cylindical-lenses-now or set the environmental variable GNU_DIFF_REASONABLE_LIMIT to a new limit, using 0 for none"
Lastly, for what it's worth from a perfect stranger:
I'm very impressed that diff didn't start consuming huge amounts of memory, and a little saddened that it is impressive!
Thanks very much for diff and your work on it, you have no idea how many things it underpins!
|[Prev in Thread]||Current Thread||[Next in Thread]|