Hello there.
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:
Line 284:
uintmax_t numval;
Line 525:
case 'W':
numval = strtoumax (optarg, &numend, 10);
if (! (0 < numval && numval <= SIZE_MAX) || *numend)
try_help ("invalid width '%s'", optarg);
if (width != numval)
{
if (width)
fatal ("conflicting width options");
width = numval;
}
break;
For convenience:
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.
ABUSE POTENTIAL:
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
SUGGESTIONS:
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!
|
0001-diff-don-t-mistreat-N-in-arg-as-a-large-number.patch