Page MenuHomeFreeBSD

lib/libc/tests/string: improve memccpy "bounds" unit test
ClosedPublic

Authored by fuz on Jul 20 2024, 4:08 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 5, 10:39 AM
Unknown Object (File)
Mon, Nov 4, 12:55 AM
Unknown Object (File)
Fri, Nov 1, 1:22 AM
Unknown Object (File)
Tue, Oct 29, 1:09 PM
Unknown Object (File)
Sun, Oct 27, 6:20 PM
Unknown Object (File)
Mon, Oct 21, 7:29 AM
Unknown Object (File)
Mon, Oct 21, 6:28 AM
Unknown Object (File)
Mon, Oct 21, 6:28 AM
Subscribers

Details

Summary

The purpose of the "bounds" test is to check that the function does not
overread the array bounds. The old unit test, copied from the strlcpy()
one, always ensured that we see the character c memccpy() is looking for
in the source array before the array ends. While this is correct for
strlcpy(), memccpy()'s specification does not guarantee that the c is
present within the given size limit.

The updated test handles this case better, ensuring that the source
array ends early if c is not supposed to be present.

With this updated test, a bug @getz found in my "baseline" memccpy
implementation now leads to the unit test failing with a crash.

The bug is as follows: if memccpy processes an input array that starts
no more than 16 bytes before the end of a page followed by an unmapped
or inaccessible page and does not cross the page boundary and if the
character we are looking for does not appear in the array, memccpy
errorneously reads another 16 bytes off the unmapped page, causing a
crash. A fix will follow in a second DR.

Test Plan

This is the test plan for an upcoming bug fix.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

fuz requested review of this revision.Jul 20 2024, 4:08 PM

Now that D46052 has been committed, could we perhaps get this unit test in, too?

lib/libc/tests/string/memccpy_test.c
66–80

guard_at_end could be a bool

194–195

OOB write on last iteration?

lib/libc/tests/string/memccpy_test.c
66–80

I guess it could, I just use int out of habit.

194–195

We only iterate to sizeof(buf) - 1 so it all stays in bound.

emaste added inline comments.
lib/libc/tests/string/memccpy_test.c
102

Only other comment, maybe MIN(bufsize, size)?

194–195

Oh yes somehow I didn't see the - 1 there

This revision is now accepted and ready to land.Sep 14 2024, 12:34 PM
lib/libc/tests/string/memccpy_test.c
102

Oh, I wasn't aware of the MIN macro.

Will add that on commit.