bsdgrep(1): Pet clang-scan

[kevans]

Summary

clang-scan reports a couple of minor issues with bsdgrep(1), so pet it. It complains
about usage of strcpy, some cases of sizeof being used in conjunction with malloc and not
using the proper type inside (unsigned int vs. int), and a potential memory leak.

The first two are relatively harmless, but probably technically correct. The memory leak could
have prove problematic given a set of valid patterns and then a pattern that consumed a lot of
memory; ultimately, probably also not a practical concern.

Mentioned by: pfg@

Test Plan

Run kyua tests to ensure no regressions

[cem]

usr.bin/grep/regex/tre-fastmatch.c

356

This might be the compiler being a little too pedantic. Can the size of int and unsigned int really differ?

I usually use sizeof(*fg->sbmGs) so the type is always correct relative to the pointer.

460–463

I'd rather SAVE_PATTERN() be implemented as a function tre_strndup(), in the style of strndup(3). Then the free() (and error return) will be handled in the callers.

usr.bin/grep/util.c

233–234

This clang-scan warning is bogus.

But: this could just be: pc.ln.file = strdup(fn);

[kevans]

• Address concerns by cem@

[kevans]

usr.bin/grep/regex/tre-fastmatch.c

356

I'm fairly certain they can't differ. =)

That's fair -- that does look+work better.

460–463

+1; I implemented this as tre_strndup. Originally I was trying to match the current usage with minimal changes, but I'm not sure if it's worth it -- naming SAVE_PATTERN_FREESRC was painful.

1082

It occurs to me now that I broke the broken style in this file here -- should I fix this, or is it OK since it's off on its own?

usr.bin/grep/util.c

233–234

IMO, the only non-bogus one was the memory leak, and that was still mostly bogus and not a practical concern. =p

I'm ashamed to admit, though, that I hadn't noticed strdup(3) before. This is fixed.

[cem]

LGTM modulo nits

usr.bin/grep/regex/tre-fastmatch.c

1082

Probably best to fix it. :-)

1090

To mirror strndup this would be strncpy(). I don't think it really matters here, though.

[kevans]

usr.bin/grep/regex/tre-fastmatch.c

1090

D'oh, now that you mention it the name of this function's technically wrong. tre_char_t is actually a wchar_t, but we don't have a matching wcsndup implementation so I guess it's alright?

I think it might be better to leave it as-is (memcpy) to hide that implementation detail, though. It is theoretically possible to compile the tre bits using multi-byte strings instead of wide char (!TRE_WCHAR), which would complicate this possibly a bit more than it needs to be? OTOH, it's not practically possible to compile it with !TRE_WCHAR without some changes to the glue.

[cem]

usr.bin/grep/regex/tre-fastmatch.c

1090

Oh, right, strncpy would be wrong for wchar strings. Yeah, I'm ok just leaving it as memcpy for now.

[kevans]

• Match indentation of the rest of the file

[kevans]

usr.bin/grep/regex/tre-fastmatch.c

1090

Alrighty. =) I'm still chomping at the bit to disable tre by default, so I'm trying not to do much in these parts.

[cem]

usr.bin/grep/regex/tre-fastmatch.c

1090

That's totally fair :-)

[cem]

Following up, as this one is still open and it's been a while.

Static analysis cleanups are ok as long as they make sense. In general I am leery of changing the tree to suite scan-build, as it is incredibly noisy (about 300-400 mostly false positives against libc, for example).

I think Coverity is a higher quality static analysis tool — mainly because it provides a persistent suppression database outside of the code — and also because it produces more true positives and fewer false positives, so I'd encourage focusing on that instead of scan-build in the future.