Page MenuHomeFreeBSD

libc/resolv: attempt to fix the test under WARNS=6
ClosedPublic

Authored by kevans on May 14 2020, 7:54 PM.

Details

Summary

In a side-change that I'm working on to start defaulting src builds to WARNS=6 where WARNS isn't otherwise specified, GCC6 (and clang, to a lesser extent) pointed out a number of issues with the resolv tests:

  • Global method variable that gets shadowed in run_tests()
  • Pointer comparison to char ('\0')
  • Signed/unsigned comparison between i in run_tests() and hosts->sl_cur

The shadowed variable looks like it might actually be bogus as written, as we pass it to RUN_TESTS -> run_tests, but other parts use the global method instead. This change is mainly geared towards correcting that by removing the global and plumbing the method through from run_tests -> run into the new thread.

I assume from the context of ptr == '\0' that it was intended to check for an empty string, so it's adjusted to ptr[0] == '\0'.

For signed/unsigned comparison, there's no compelling reason to not just switch i/nthreads/nhosts to size_t.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ngie accepted this revision.EditedMay 14 2020, 8:22 PM

Thank you!

surewhynot

This revision is now accepted and ready to land.May 14 2020, 8:22 PM
pstef added inline comments.
lib/libc/tests/resolv/resolv_test.c
84 ↗(On Diff #71797)

I think this was checking for a null pointer, in which case you risk a null pointer dereference after this change.

lib/libc/tests/resolv/resolv_test.c
84 ↗(On Diff #71797)

ptr is already checked for null as part of the loop invariant here.

pstef added inline comments.
lib/libc/tests/resolv/resolv_test.c
84 ↗(On Diff #71797)

I admit I was wrong about the risk. But I'm not convinced it wasn't a bells and whistles... sorry, belts and suspenders kind of safety guarantee. I know I've written such reduntant checks. It's difficult for me to imagine strtok() returning an empty string, but I'm not sure.

This revision was automatically updated to reflect the committed changes.