Page MenuHomeFreeBSD

Improve test coverage for sbuf
Needs RevisionPublic

Authored by pnagato_protonmail.com on Aug 28 2020, 5:33 PM.

Details

Reviewers
jmg
ngie
Summary

Added the following tests

  • sbuf_clear_flags_test
  • sbuf_set_get_flags_test

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 33587
Build 30838: arc lint + arc unit

Event Timeline

@jmg @imp Need your input how to proceed on the sbuf_negative test.

@jmg @imp Need your input how to proceed on the sbuf_negative test.

I'm having trouble understanding its purpose. Maybe you could give a one or two sentence summary of what it should test?

In D26220#584521, @imp wrote:

@jmg @imp Need your input how to proceed on the sbuf_negative test.

I'm having trouble understanding its purpose. Maybe you could give a one or two sentence summary of what it should test?

@imp I am assuming the sbuf_new_negative_test is supposed to test for cases for which sbuf_new fails to create a sbuf.

From sys/kern/subr_sbuf.c

	KASSERT(length >= 0,
	    ("attempt to create an sbuf of negative length (%d)", length));
	KASSERT((flags & ~SBUF_USRFLAGMSK) == 0,
	    ("%s called with invalid flags", __func__));

or when SBMALLOC fails..

In D26220#584521, @imp wrote:

@jmg @imp Need your input how to proceed on the sbuf_negative test.

I'm having trouble understanding its purpose. Maybe you could give a one or two sentence summary of what it should test?

@imp I am assuming the sbuf_new_negative_test is supposed to test for cases for which sbuf_new fails to create a sbuf.

From sys/kern/subr_sbuf.c

	KASSERT(length >= 0,
	    ("attempt to create an sbuf of negative length (%d)", length));
	KASSERT((flags & ~SBUF_USRFLAGMSK) == 0,
	    ("%s called with invalid flags", __func__));

or when SBMALLOC fails..

So, looking at the KASSERT, we may want to change how KASSERTs are compiled for userland. Right now it gets compiled to nothing, which means that there is nothing preventing userland from passing negative lengths and the like. We may want to think about making KASSERT call abort, but then some KASSERTs (like those in sbuf_new quoted above) need to be changed because aborting when length, which might be a user defined parameter seems like a bad idea.

Also, it looks like sbuf.9 doesn't document the KASSERT restrictions in sbuf_new, so a userland caller may accidentally generate a corrupted sbuf.

As for how to make sbuf fail due to malloc, I don't know.

Skip sbuf_new_negative_test,implement sbuf_new_positive_test

In D26220#584584, @jmg wrote:
In D26220#584521, @imp wrote:

@jmg @imp Need your input how to proceed on the sbuf_negative test.

I'm having trouble understanding its purpose. Maybe you could give a one or two sentence summary of what it should test?

@imp I am assuming the sbuf_new_negative_test is supposed to test for cases for which sbuf_new fails to create a sbuf.

From sys/kern/subr_sbuf.c

	KASSERT(length >= 0,
	    ("attempt to create an sbuf of negative length (%d)", length));
	KASSERT((flags & ~SBUF_USRFLAGMSK) == 0,
	    ("%s called with invalid flags", __func__));

or when SBMALLOC fails..

So, looking at the KASSERT, we may want to change how KASSERTs are compiled for userland. Right now it gets compiled to nothing, which means that there is nothing preventing userland from passing negative lengths and the like. We may want to think about making KASSERT call abort, but then some KASSERTs (like those in sbuf_new quoted above) need to be changed because aborting when length, which might be a user defined parameter seems like a bad idea.

Also, it looks like sbuf.9 doesn't document the KASSERT restrictions in sbuf_new, so a userland caller may accidentally generate a corrupted sbuf.

As for how to make sbuf fail due to malloc, I don't know.

@jmg i will skip the sbuf_new_negative_test for now. Please review the other tests.

In D26220#584584, @jmg wrote:
In D26220#584521, @imp wrote:

@jmg @imp Need your input how to proceed on the sbuf_negative test.

I'm having trouble understanding its purpose. Maybe you could give a one or two sentence summary of what it should test?

@imp I am assuming the sbuf_new_negative_test is supposed to test for cases for which sbuf_new fails to create a sbuf.

From sys/kern/subr_sbuf.c

	KASSERT(length >= 0,
	    ("attempt to create an sbuf of negative length (%d)", length));
	KASSERT((flags & ~SBUF_USRFLAGMSK) == 0,
	    ("%s called with invalid flags", __func__));

or when SBMALLOC fails..

So, looking at the KASSERT, we may want to change how KASSERTs are compiled for userland. Right now it gets compiled to nothing, which means that there is nothing preventing userland from passing negative lengths and the like. We may want to think about making KASSERT call abort, but then some KASSERTs (like those in sbuf_new quoted above) need to be changed because aborting when length, which might be a user defined parameter seems like a bad idea.

Also, it looks like sbuf.9 doesn't document the KASSERT restrictions in sbuf_new, so a userland caller may accidentally generate a corrupted sbuf.

As for how to make sbuf fail due to malloc, I don't know.

@jmg i will skip the sbuf_new_negative_test for now. Please review the other tests.

@jmg @imp I have seen other places where KASSERT was re-defined as

#define	KASSERT(exp,msg) assert((exp))

Another way
http://fxr.watson.org/fxr/source/netpfil/ipfw/test/dn_test.h#L143

#define KASSERT(x, y)   do { if (!(x)) printf y ; exit(0); } while (0)

for the tests. Please let me know if we can go that route.

@jmg @imp I have seen other places where KASSERT was re-defined as

#define	KASSERT(exp,msg) assert((exp))

I think using assert should be used. It doesn't pollute stdout, generates a core dump, and is more obvious that there was a failure.

Thanks.

In D26220#588231, @jmg wrote:

@jmg @imp I have seen other places where KASSERT was re-defined as

#define	KASSERT(exp,msg) assert((exp))

I think using assert should be used. It doesn't pollute stdout, generates a core dump, and is more obvious that there was a failure.

Thanks.

I think using the other one is better since we'll get the message. assert doesn't give the message:

#define KASSERT(x, y)   do { if (!(x)) printf y ; exit(1); } while (0)

though maybe 'abort' would be better. exit(0) is bad because the test won't be considered failed, perhaps...

@imp

atf_tc_expect_death("....") will print the reason. For e.g.

sbuf_core_test:sbuf_new_negative_test  ->  expected_failure: Buffer length cannot be negative  [0.008s]
sbuf_core_test:sbuf_new_negative_test_min_buf_size  ->  expected_failure: Minimum buffer length should be 2  [0.008s]
sbuf_core_test:sbuf_new_negative_test_non_user_flags  ->  expected_failure: Non user flags cannot be specified  [0.008s]
sbuf_core_test:sbuf_setpos_test_negative_pos  ->  expected_failure: position cannot be negative  [0.008s]

@imp if the test binary is run directly like this. We can see the assertion output.

./sbuf_core_test sbuf_new_negative_test_non_user_flags

sbuf_core_test: WARNING: Running test cases outside of kyua(1) is unsupported
sbuf_core_test: WARNING: No isolation nor timeout control is being applied; you may get unexpected failures; see atf-test-case(4)
expected_death: Non user flags cannot be specified
Assertion failed: (((flags & ~0x0000ffff) == 0)), function sbuf_new, file /usr/src/sys/kern/subr_sbuf.c, line 233.
Abort (core dumped)

@imp if the test binary is run directly like this. We can see the assertion output.

./sbuf_core_test sbuf_new_negative_test_non_user_flags

sbuf_core_test: WARNING: Running test cases outside of kyua(1) is unsupported
sbuf_core_test: WARNING: No isolation nor timeout control is being applied; you may get unexpected failures; see atf-test-case(4)
expected_death: Non user flags cannot be specified
Assertion failed: (((flags & ~0x0000ffff) == 0)), function sbuf_new, file /usr/src/sys/kern/subr_sbuf.c, line 233.
Abort (core dumped)

@imp tried the do while.. & assert. In both cases kyua does not print to log file. I see the msg only when the binary is run standalone or a single test case is run. For e.g.

kyua debug sbuf_core_test:sbuf_new_negative_test
Assertion failed: ((length >= 0)), function sbuf_new, file /usr/src/sys/kern/subr_sbuf.c, line 231.
Process with PID 29505 exited with signal 6 and dumped core; attempting to gather stack trace
Cannot find GDB binary; builtin was 'gdb'
sbuf_core_test:sbuf_new_negative_test  ->  expected_failure: Buffer length cannot be negative

@imp I removed the output from the kyua log from this comment.

@imp if the test binary is run directly like this. We can see the assertion output.

./sbuf_core_test sbuf_new_negative_test_non_user_flags

sbuf_core_test: WARNING: Running test cases outside of kyua(1) is unsupported
sbuf_core_test: WARNING: No isolation nor timeout control is being applied; you may get unexpected failures; see atf-test-case(4)
expected_death: Non user flags cannot be specified
Assertion failed: (((flags & ~0x0000ffff) == 0)), function sbuf_new, file /usr/src/sys/kern/subr_sbuf.c, line 233.
Abort (core dumped)

@imp tried the do while.. & assert. In both cases kyua does not print to log file. I see the msg only when the binary is run standalone or a single test case is run. For e.g.

kyua debug sbuf_core_test:sbuf_new_negative_test
Assertion failed: ((length >= 0)), function sbuf_new, file /usr/src/sys/kern/subr_sbuf.c, line 231.
Process with PID 29505 exited with signal 6 and dumped core; attempting to gather stack trace
Cannot find GDB binary; builtin was 'gdb'
sbuf_core_test:sbuf_new_negative_test  ->  expected_failure: Buffer length cannot be negative

@imp I removed the output from the kyua log from this comment.

@jmg Please review.

ngie requested changes to this revision.EditedOct 30 2020, 7:49 PM

This change does more than advertised in the CR description (which doesn't describe as much as the commit message should).

  • How was it determined that these areas are missing coverage? Code inspection? gcov?
  • This changes the behavior of libsbuf by asserting on exceptional cases that can be triggered by humans. Also, compiling libsbuf with MK_ASSERT_DEBUG==no will completely invalidate the assertions breaking the test and triggering unwanted behavior where KASSERT was used. Libraries with publicly moving parts shouldn't assert (especially on invalid input); they should return sensible errors which are documented in the manpages which describe the APIs.
lib/libsbuf/tests/sbuf_core_test.c
109–118

Please use __unused.

150–164

Why were these invariants removed?

185

Use _exit instead of exit.

This avoids the atexit handlers which may have been installed earlier on in the application or library:

EXIT(2)                   FreeBSD System Calls Manual                  EXIT(2)

NAME
     _exit – terminate the calling process

LIBRARY
     Standard C Library (libc, -lc)

SYNOPSIS
     #include <unistd.h>

     void
     _exit(int status);

DESCRIPTION
     The _exit() system call terminates a process with the following
     consequences:

     •   All of the descriptors open in the calling process are closed.  This
         may entail delays, for example, waiting for output to drain; a
         process in this state may not be killed, as it is already dying.

     •   If the parent process of the calling process has an outstanding
         wait(2) call or catches the SIGCHLD signal, it is notified of the
         calling process's termination and the status is set as defined by
         wait(2).

     •   The parent process-ID of all of the calling process's existing child
         processes are set to the process-ID of the calling process's reaper;
         the reaper (normally the initialization process) inherits each of
         these processes (see procctl(2), init(8) and the DEFINITIONS section
         of intro(2)).

     •   If the termination of the process causes any process group to become
         orphaned (usually because the parents of all members of the group
         have now exited; see “orphaned process group” in intro(2)), and if
         any member of the orphaned group is stopped, the SIGHUP signal and
         the SIGCONT signal are sent to all members of the newly-orphaned
         process group.

     •   If the process is a controlling process (see intro(2)), the SIGHUP
         signal is sent to the foreground process group of the controlling
         terminal, and all current access to the controlling terminal is
         revoked.

     Most C programs call the library routine exit(3), which flushes buffers,
     closes streams, unlinks temporary files, etc., before calling _exit().
sys/kern/subr_sbuf.c
46–63

This seems like it should be a separate change, maybe. Also, have all of the code paths been analyzed to ensure there aren't dead code paths after this change with libsbuf?

This revision now requires changes to proceed.Oct 30 2020, 7:49 PM

@ngie @lwhsu Hello, I found this on https://wiki.freebsd.org/JuniorJobs, Please close this. This is more involved than what i could comprehend from that page.