bsdgrep(1): Don't allow negative -A/-B/-C

[kevans]

Summary

Current bsdgrep(1) behavior when given a negative -A/-B/-C argument is to overflow the
respective context flag(s) and thus exhibit surprising behavior. Fix this by removing
unsignedness of Aflag/Bflag and erroring out if we're given a value < 0. Also adjust the type
used to track 'tail' context in procfile() so that it accurately reflects the Aflag value rather than overflowing
and losing trailing context.

This also fixes an inconsistency previously existing between -n and -C "n" behavior. The former
was previously limited to less than ULLONG_MAX, while the latter could attain up to ULLONG_MAX.
They are now both limited to LLONG_MAX, to be consistent.

For this, we add some test cases to make sure grep errors out properly for both negative context values as well
as non-numeric context values rather than giving bogus matches.

Test Plan

Ensure no regressions in Kyua tests

[kevans]

• Adjust 'tail' type to match Aflag type, to fix other inconsistent behavior. Specifically, when Aflag's value actually overflows an int.

[cem]

usr.bin/grep/grep.c

442–443 ↗

(On Diff #28223)

I think the previous check was actually more correct.

Signed overflow is undefined behavior in C. To get the compiler to treat it as you expect, you would need to explicitly build bsdgrep with -fwrapv. And it's better to avoid introducing that need.

459–460 ↗

(On Diff #28223)

Why check l values if we already observed an errno?

464 ↗

(On Diff #28223)

Maybe: err(2, "context argument must be non-negative");?

[kevans]

usr.bin/grep/grep.c

442–443 ↗

(On Diff #28223)

More correct, but still wrong in that it should've checked for it following the Aflag adjustment rather than pre-adjustment and LLONG_MAX/10.

A previous version of my change checked Aflag < 0 || Aflag > LLONG_MAX, but I wasn't sure if that would be immediately frowned upon given Aflag being a long long -- I'll re-add this?

459–460 ↗

(On Diff #28223)

I'm not privy to the original details, but I think it was to sufficiently detect overflow/underflow as a result of strtoll -- it's defined to clamp the return to LLONG_MIN/LLONG_MAX to indicate underflow/overflow, *if* strtoll sets ERANGE.

I have no idea why it's checked for EINVAL, though -- strtol(3) does not seem to indicate any kind of useful return value in that case.

464 ↗

(On Diff #28223)

I think I was avoiding that, but I don't recall why, so I'll fix it. =)

[cem]

usr.bin/grep/grep.c

442–443 ↗

(On Diff #28223)

Don't you have to check for it pre-adjustment, or you may have overflowed?

If we get to this case, we know we are going to multiply by ten. So comparing against LLONG_MAX/10 makes sense to me.

Why check for Aflag < 0 at all? It is impossible (by the C standard). And Aflag > LLONG_MAX is also impossible.

459–460 ↗

(On Diff #28223)

I think the l checks should just be dropped — errno is sufficient and more clear.

[kevans]

usr.bin/grep/grep.c

442–443 ↗

(On Diff #28223)

Indeed, but in every practical situation I've encountered overflow is represented in one of these two ways.

The previous pre-overflow check had some weird behavior to it -- I'll double check that, though.

[cem]

usr.bin/grep/grep.c

442–443 ↗

(On Diff #28223)

Modern C compilers will often do perverse things when you make undefined checks like this (the < 0 one). The least harmful thing they'll do is just remove the check entirely. It can be worse than that, though.

The > LLONG_MAX check will generate a warning and be optimized out. It's defined, just tautalogical.

Not sure we care too much around weird behaviors with 2**63-1 lines of context :-), but it would be good to fix the check that will actually work.

[kevans]

usr.bin/grep/grep.c

442–443 ↗

(On Diff #28223)

I'll restore it- my main concern is when AFlag == LLONG_MAX / 10 and c - '0' == LLONG_MAX % 10 -- that, too, will result in an overflow, so I will likely add in that condition. It bothers me generally because overflowing the Aflag in particular ends up printing no -A context rather than all the -A context -- this isn't intuitive when you've specified a large number.

442–443 ↗

(On Diff #28223)

Sorry, > LLONG_MAX % 10

[cem]

usr.bin/grep/grep.c

442–443 ↗

(On Diff #28223)

We could just check: if (AFlag > LLONG_MAX / 10 - 1). Sure, there are technically some numbers in there that don't overflow, but, I really do not think it matters.

[kevans]

usr.bin/grep/grep.c

442–443 ↗

(On Diff #28223)

*nod*

I'm more concerned about the overflow chopping off tail context than I am not allowing that much context -- error is OK, deceiving results not so much.

[kevans]

• Restore previous pre-check for overflow in Aflag, cut off -n context settings *before* we hit overflow, rather than after, so as to not cut off trailing context giving more difficult-to-detect erroneous behavior
• Simplify 'errno' check following strtoll
• Add a sensible error message for negative -A/-B/-C values

[cem]

usr.bin/grep/grep.c

459 ↗

(On Diff #28253)

Would errno != 0 be a problem? :-)

[kevans]

usr.bin/grep/grep.c

459 ↗

(On Diff #28253)

Heh, don't see why it would be. =) I was trying to do minimal damage to these lines, though, since my original change to this bit was just s/ULLONG_MAX/LLONG_MAX/

[emaste]

usr.bin/grep/grep.c

111–112 ↗

(On Diff #28253)

These should retain the existing whitespace formatting.

[kevans]

usr.bin/grep/grep.c

111–112 ↗

(On Diff #28253)

? I think Phabricator is displaying this strangely (wrong) here- as it is, these comments now line up with all of the other ones using a single tab. The diff downloaded from Phabricator also shows how it should be, I think.

[emaste]

usr.bin/grep/grep.c

111–112 ↗

(On Diff #28253)

Ah yes.

Aside, I think Phabricator is displaying these lines correctly (there's a single space between long and Aflag) but the bools below are tab-space and are indented strangely.

[kevans]

usr.bin/grep/grep.c

111–112 ↗

(On Diff #28253)

Right, ok, that makes sense. Anything I need to adjust around here, then? The formatting I ended up with matches mcount and mlimit further down, also long long -- none of these have any hope of lining up with 'color' down below without more spaces, so I'm not sure if there's a good way to handle this =(