Details

Reviewers

asomers
rwatson

Commits

rS337256: MFC r335287, r335290
rS336212: MFC r335287, r335290
rS335290: praudit(1): add tests

Summary

The revision introduces a new test-program for an audit viewer application: praudit.
The tests have been created for checking the proper functioning of each mode of execution.

-d del  Specifies the delimiter.  The default delimiter is the comma.

-l      Prints the entire record on the same line.  If this option is not
        specified, every token is displayed on a different line.

-n      Do not convert user and group IDs to their names but leave in
        their numeric forms.

-p      Specify this option if input to praudit is piped from the tail(1)
        utility.  This causes praudit to sync to the start of the next
        record.

-r      Prints the records in their raw, numeric form.  This option is
        exclusive from -s.

-s      Prints the tokens in their short form.  Short text
        representations for record and event type are displayed.  This
        option is exclusive from -r.

-x      Print audit records in the XML output format.

NOTE: The above snippet is taken from praudit.1

Test Plan

Execute make install from usr.sbin/praudit/tests.
Execute kyua test from /usr/test/usr.sbin/praudit. The test case should succeed.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 17252
Build 17095: arc lint + arc unit

Event Timeline

aniketp created this revision.Jun 10 2018, 11:36 PM

Harbormaster completed remote builds in B17156: Diff 43574.Jun 10 2018, 11:36 PM

What about -p? And how about asserting the exclusivity of -s and -r in a negative test?

usr.sbin/praudit/tests/Makefile
19	Remove the final backslash
usr.sbin/praudit/tests/praudit_test.sh
34	As these assertions are all independent, you should split them into separate test cases.
38	Don't assert that the exit status is 1. That's a bug. Fix the bug instead.

This revision now requires changes to proceed.Jun 11 2018, 12:16 AM

Add a test for praudit exclusivity for 'r' and 's' arguments
Separate the test cases from one another

Harbormaster completed remote builds in B17160: Diff 43580.Jun 11 2018, 2:25 AM

Update the praudit_raw_short_exclusive's description

Harbormaster completed remote builds in B17161: Diff 43581.Jun 11 2018, 2:29 AM

Replace the audit startup trail with socket(2) trail
Improvements in the raw_short_exclusive description

Harbormaster completed remote builds in B17168: Diff 43592.Jun 11 2018, 10:32 AM

I still think you should try to test "-p". That's the only thing standing between you and full coverage of the praudit command line.

Add a test case for -p option which is used with tail(1)

Harbormaster completed remote builds in B17222: Diff 43691.Jun 13 2018, 6:07 AM

Add a test case for -p option which is used with tail(1)

I had missed the corrupted record earlier

Harbormaster completed remote builds in B17223: Diff 43692.Jun 13 2018, 6:09 AM

Update praudit's flag '-p': Earlier the tests were failing

Harbormaster completed remote builds in B17224: Diff 43694.Jun 13 2018, 6:48 AM

Update praudit_sync_to_next_record test case

Harbormaster completed remote builds in B17225: Diff 43695.Jun 13 2018, 6:56 AM

asomers added inline comments.Jun 13 2018, 3:25 PM

usr.sbin/praudit/tests/praudit_test.sh
149	Is it a requirement that praudit _can't_ work with a truncated record without `-p`? I don't think it is. So you should probably remove the first assertion. Also, could you somewhere include a textual description of the difference between the normal and the truncated records?

aniketp marked 3 inline comments as done.Jun 13 2018, 3:40 PM

aniketp added inline comments.

usr.sbin/praudit/tests/praudit_test.sh
149	@asomers I had added extra random texts in the trail (before correct audit record) and verified that `praudit trail` resulted in empty STDOUT while `praudit -p trail` gave only the correct (later) part of the stdout.

Remove the normal praudit atf_check assertion in the praudit_sync_to_next_record test-case

Harbormaster completed remote builds in B17252: Diff 43741.Jun 14 2018, 3:19 AM

I would still like to see a comment describing the contents of the corrupted file.

Add a description on the intent of testing the -p option

Harbormaster completed remote builds in B17274: Diff 43769.Jun 14 2018, 4:29 PM

asomers added inline comments.Jun 15 2018, 2:27 PM

usr.sbin/praudit/tests/praudit_test.sh
150	I doesn't read from the end. It still reads from the front; but it syncs to the start of the first whole record before it does anything else.

Update the test case description

Harbormaster completed remote builds in B17306: Diff 43829.Jun 15 2018, 4:42 PM

aniketp retitled this revision from Add a single test-case for praudit(1) utility to Add a single test-program for praudit(1) utility.Jun 15 2018, 4:44 PM

aniketp set the repository for this revision to rS FreeBSD src repository - subversion.

Herald added a subscriber: imp. · View Herald TranscriptJun 15 2018, 4:44 PM

The praudit_test:praudit_sync_to_next_record spins the CPU and never returns. Does it do that for you too?

No @asomers ,
In its current form it passes smoothly:

praudit_test:praudit_sync_to_next_record  ->  passed  [0.027s]

Since praudit -p occasionally spins the cpu (as confirmed by Aniket on IRC) you should set a shorter timeout. Apart from that this review looks good. However, it does depend on the change to praudit's return code, which hasn't been committed yet.

This revision now requires changes to proceed.Jun 16 2018, 2:18 PM

No @asomers, praudit -p never spins the CPU in normal invocation. (Apologies if my earlier statement was unclear)
This is the observation:

Case 1) For the tail(1) usage:

	atf_check -o file:$(atf_get_srcdir)/no_args \
		tail $(atf_get_srcdir)/corrupted | praudit -p

CPU usage shoots up and test case never returns.

Case 2) For the normal usage:

	atf_check -o file:$(atf_get_srcdir)/no_args \
		praudit -p $(atf_get_srcdir)/corrupted

Result is successful everytime.

Reduce the timeout to 2 seconds

Harbormaster completed remote builds in B17345: Diff 43902.Jun 16 2018, 3:47 PM

I figured out why it works for you but not for me; it's arcanist's fault. Arcanist apparently doesn't like binary files. When I do "arc patch" it creates corrupted and trail as 0-length files. That means that a repro case for the bug is "touch /tmp/empty; praudit -p /tmp/empty".

I'm accepting this revision, but it still depends on fixing praudit's exit status.

This revision is now accepted and ready to land.Jun 17 2018, 5:01 PM

Closed by commit rS335290: praudit(1): add tests (authored by asomers). · Explain WhyJun 17 2018, 5:31 PM

This revision was automatically updated to reflect the committed changes.

Diff 43741

View Options

etc/mtree/BSD.tests.dist

Show First 20 Lines • Show All 73 Lines • ▼ Show 20 Lines
#include "opt_vm.h"		#include "opt_vm.h"

#include <sys/param.h>		#include <sys/param.h>
#include <sys/bio.h>		#include <sys/bio.h>
#include <sys/blist.h>		#include <sys/blist.h>
#include <sys/buf.h>		#include <sys/buf.h>
#include <sys/conf.h>		#include <sys/conf.h>
#include <sys/disk.h>		#include <sys/disk.h>
		#include <sys/disklabel.h>
#include <sys/eventhandler.h>		#include <sys/eventhandler.h>
#include <sys/fcntl.h>		#include <sys/fcntl.h>
#include <sys/lock.h>		#include <sys/lock.h>
#include <sys/kernel.h>		#include <sys/kernel.h>
#include <sys/mount.h>		#include <sys/mount.h>
#include <sys/namei.h>		#include <sys/namei.h>
#include <sys/malloc.h>		#include <sys/malloc.h>
#include <sys/pctrie.h>		#include <sys/pctrie.h>
▲ Show 20 Lines • Show All 2,203 Lines • ▼ Show 20 Lines	swaponsomething(struct vnode vp, void id, u_long nblks,
sp->sw_nblks = nblks;		sp->sw_nblks = nblks;
sp->sw_used = 0;		sp->sw_used = 0;
sp->sw_strategy = strategy;		sp->sw_strategy = strategy;
sp->sw_close = close;		sp->sw_close = close;
sp->sw_flags = flags;		sp->sw_flags = flags;

sp->sw_blist = blist_create(nblks, M_WAITOK);		sp->sw_blist = blist_create(nblks, M_WAITOK);
/*		/*
* Do not free the first two block in order to avoid overwriting		* Do not free the first blocks in order to avoid overwriting
* any bsd label at the front of the partition		* any bsd label at the front of the partition
*/		*/
blist_free(sp->sw_blist, 2, nblks - 2);		blist_free(sp->sw_blist, howmany(BBSIZE, PAGE_SIZE),
		nblks - howmany(BBSIZE, PAGE_SIZE));

		alcUnsubmitted Not Done Inline Actions Consider the possibility that PAGE_SIZE is greater than BBSIZE. (We still barely support one architecture, sparc64, where they are equal.) I think that howmany(BBSIZE, PAGE_SIZE) will "do the right thing." alc: Consider the possibility that PAGE_SIZE is greater than BBSIZE. (We still barely support one…
dvbase = 0;		dvbase = 0;
mtx_lock(&sw_dev_mtx);		mtx_lock(&sw_dev_mtx);
TAILQ_FOREACH(tsp, &swtailq, sw_list) {		TAILQ_FOREACH(tsp, &swtailq, sw_list) {
if (tsp->sw_end >= dvbase) {		if (tsp->sw_end >= dvbase) {
/*		/*
* We put one uncovered page between the devices		* We put one uncovered page between the devices
* in order to definitively prevent any cross-device		* in order to definitively prevent any cross-device
* I/O requests		* I/O requests
*/		*/
dvbase = tsp->sw_end + 1;		dvbase = tsp->sw_end + 1;
}		}
}		}
sp->sw_first = dvbase;		sp->sw_first = dvbase;
sp->sw_end = dvbase + nblks;		sp->sw_end = dvbase + nblks;
TAILQ_INSERT_TAIL(&swtailq, sp, sw_list);		TAILQ_INSERT_TAIL(&swtailq, sp, sw_list);
nswapdev++;		nswapdev++;
swap_pager_avail += nblks - 2;		swap_pager_avail += nblks - howmany(BBSIZE, PAGE_SIZE);
		alcUnsubmitted Not Done Inline Actions This should also use howmany(BBSIZE, PAGE_SIZE). alc: This should also use howmany(BBSIZE, PAGE_SIZE).
swap_total += nblks;		swap_total += nblks;
swapon_check_swzone();		swapon_check_swzone();
swp_sizecheck();		swp_sizecheck();
mtx_unlock(&sw_dev_mtx);		mtx_unlock(&sw_dev_mtx);
EVENTHANDLER_INVOKE(swapon, sp);		EVENTHANDLER_INVOKE(swapon, sp);
}		}

/*		/*
▲ Show 20 Lines • Show All 661 Lines • Show Last 20 Lines

View Options

usr.sbin/praudit/Makefile

Show First 20 Lines • Show All 73 Lines • ▼ Show 20 Lines
#include "opt_vm.h"		#include "opt_vm.h"

#include <sys/param.h>		#include <sys/param.h>
#include <sys/bio.h>		#include <sys/bio.h>
#include <sys/blist.h>		#include <sys/blist.h>
#include <sys/buf.h>		#include <sys/buf.h>
#include <sys/conf.h>		#include <sys/conf.h>
#include <sys/disk.h>		#include <sys/disk.h>
		#include <sys/disklabel.h>
#include <sys/eventhandler.h>		#include <sys/eventhandler.h>
#include <sys/fcntl.h>		#include <sys/fcntl.h>
#include <sys/lock.h>		#include <sys/lock.h>
#include <sys/kernel.h>		#include <sys/kernel.h>
#include <sys/mount.h>		#include <sys/mount.h>
#include <sys/namei.h>		#include <sys/namei.h>
#include <sys/malloc.h>		#include <sys/malloc.h>
#include <sys/pctrie.h>		#include <sys/pctrie.h>
▲ Show 20 Lines • Show All 2,203 Lines • ▼ Show 20 Lines	swaponsomething(struct vnode vp, void id, u_long nblks,
sp->sw_nblks = nblks;		sp->sw_nblks = nblks;
sp->sw_used = 0;		sp->sw_used = 0;
sp->sw_strategy = strategy;		sp->sw_strategy = strategy;
sp->sw_close = close;		sp->sw_close = close;
sp->sw_flags = flags;		sp->sw_flags = flags;

sp->sw_blist = blist_create(nblks, M_WAITOK);		sp->sw_blist = blist_create(nblks, M_WAITOK);
/*		/*
* Do not free the first two block in order to avoid overwriting		* Do not free the first blocks in order to avoid overwriting
* any bsd label at the front of the partition		* any bsd label at the front of the partition
*/		*/
blist_free(sp->sw_blist, 2, nblks - 2);		blist_free(sp->sw_blist, howmany(BBSIZE, PAGE_SIZE),
		nblks - howmany(BBSIZE, PAGE_SIZE));

		alcUnsubmitted Not Done Inline Actions Consider the possibility that PAGE_SIZE is greater than BBSIZE. (We still barely support one architecture, sparc64, where they are equal.) I think that howmany(BBSIZE, PAGE_SIZE) will "do the right thing." alc: Consider the possibility that PAGE_SIZE is greater than BBSIZE. (We still barely support one…
dvbase = 0;		dvbase = 0;
mtx_lock(&sw_dev_mtx);		mtx_lock(&sw_dev_mtx);
TAILQ_FOREACH(tsp, &swtailq, sw_list) {		TAILQ_FOREACH(tsp, &swtailq, sw_list) {
if (tsp->sw_end >= dvbase) {		if (tsp->sw_end >= dvbase) {
/*		/*
* We put one uncovered page between the devices		* We put one uncovered page between the devices
* in order to definitively prevent any cross-device		* in order to definitively prevent any cross-device
* I/O requests		* I/O requests
*/		*/
dvbase = tsp->sw_end + 1;		dvbase = tsp->sw_end + 1;
}		}
}		}
sp->sw_first = dvbase;		sp->sw_first = dvbase;
sp->sw_end = dvbase + nblks;		sp->sw_end = dvbase + nblks;
TAILQ_INSERT_TAIL(&swtailq, sp, sw_list);		TAILQ_INSERT_TAIL(&swtailq, sp, sw_list);
nswapdev++;		nswapdev++;
swap_pager_avail += nblks - 2;		swap_pager_avail += nblks - howmany(BBSIZE, PAGE_SIZE);
		alcUnsubmitted Not Done Inline Actions This should also use howmany(BBSIZE, PAGE_SIZE). alc: This should also use howmany(BBSIZE, PAGE_SIZE).
swap_total += nblks;		swap_total += nblks;
swapon_check_swzone();		swapon_check_swzone();
swp_sizecheck();		swp_sizecheck();
mtx_unlock(&sw_dev_mtx);		mtx_unlock(&sw_dev_mtx);
EVENTHANDLER_INVOKE(swapon, sp);		EVENTHANDLER_INVOKE(swapon, sp);
}		}

/*		/*
▲ Show 20 Lines • Show All 661 Lines • Show Last 20 Lines

View Options

usr.sbin/praudit/tests/Makefile

Show First 20 Lines • Show All 73 Lines • ▼ Show 20 Lines
#include "opt_vm.h"		#include "opt_vm.h"

#include <sys/param.h>		#include <sys/param.h>
#include <sys/bio.h>		#include <sys/bio.h>
#include <sys/blist.h>		#include <sys/blist.h>
#include <sys/buf.h>		#include <sys/buf.h>
#include <sys/conf.h>		#include <sys/conf.h>
#include <sys/disk.h>		#include <sys/disk.h>
		#include <sys/disklabel.h>
#include <sys/eventhandler.h>		#include <sys/eventhandler.h>
#include <sys/fcntl.h>		#include <sys/fcntl.h>
#include <sys/lock.h>		#include <sys/lock.h>
#include <sys/kernel.h>		#include <sys/kernel.h>
#include <sys/mount.h>		#include <sys/mount.h>
#include <sys/namei.h>		#include <sys/namei.h>
#include <sys/malloc.h>		#include <sys/malloc.h>
#include <sys/pctrie.h>		#include <sys/pctrie.h>
▲ Show 20 Lines • Show All 2,203 Lines • ▼ Show 20 Lines	swaponsomething(struct vnode vp, void id, u_long nblks,
sp->sw_nblks = nblks;		sp->sw_nblks = nblks;
sp->sw_used = 0;		sp->sw_used = 0;
sp->sw_strategy = strategy;		sp->sw_strategy = strategy;
sp->sw_close = close;		sp->sw_close = close;
sp->sw_flags = flags;		sp->sw_flags = flags;

sp->sw_blist = blist_create(nblks, M_WAITOK);		sp->sw_blist = blist_create(nblks, M_WAITOK);
/*		/*
* Do not free the first two block in order to avoid overwriting		* Do not free the first blocks in order to avoid overwriting
* any bsd label at the front of the partition		* any bsd label at the front of the partition
*/		*/
blist_free(sp->sw_blist, 2, nblks - 2);		blist_free(sp->sw_blist, howmany(BBSIZE, PAGE_SIZE),
		nblks - howmany(BBSIZE, PAGE_SIZE));

		alcUnsubmitted Not Done Inline Actions Consider the possibility that PAGE_SIZE is greater than BBSIZE. (We still barely support one architecture, sparc64, where they are equal.) I think that howmany(BBSIZE, PAGE_SIZE) will "do the right thing." alc: Consider the possibility that PAGE_SIZE is greater than BBSIZE. (We still barely support one…
dvbase = 0;		dvbase = 0;
mtx_lock(&sw_dev_mtx);		mtx_lock(&sw_dev_mtx);
TAILQ_FOREACH(tsp, &swtailq, sw_list) {		TAILQ_FOREACH(tsp, &swtailq, sw_list) {
if (tsp->sw_end >= dvbase) {		if (tsp->sw_end >= dvbase) {
/*		/*
* We put one uncovered page between the devices		* We put one uncovered page between the devices
* in order to definitively prevent any cross-device		* in order to definitively prevent any cross-device
* I/O requests		* I/O requests
*/		*/
dvbase = tsp->sw_end + 1;		dvbase = tsp->sw_end + 1;
}		}
}		}
sp->sw_first = dvbase;		sp->sw_first = dvbase;
sp->sw_end = dvbase + nblks;		sp->sw_end = dvbase + nblks;
TAILQ_INSERT_TAIL(&swtailq, sp, sw_list);		TAILQ_INSERT_TAIL(&swtailq, sp, sw_list);
nswapdev++;		nswapdev++;
swap_pager_avail += nblks - 2;		swap_pager_avail += nblks - howmany(BBSIZE, PAGE_SIZE);
		alcUnsubmitted Not Done Inline Actions This should also use howmany(BBSIZE, PAGE_SIZE). alc: This should also use howmany(BBSIZE, PAGE_SIZE).
swap_total += nblks;		swap_total += nblks;
swapon_check_swzone();		swapon_check_swzone();
swp_sizecheck();		swp_sizecheck();
mtx_unlock(&sw_dev_mtx);		mtx_unlock(&sw_dev_mtx);
EVENTHANDLER_INVOKE(swapon, sp);		EVENTHANDLER_INVOKE(swapon, sp);
}		}

/*		/*
▲ Show 20 Lines • Show All 661 Lines • Show Last 20 Lines

View Options

usr.sbin/praudit/tests/input/corrupted

Show First 20 Lines • Show All 73 Lines • ▼ Show 20 Lines
#include "opt_vm.h"		#include "opt_vm.h"

#include <sys/param.h>		#include <sys/param.h>
#include <sys/bio.h>		#include <sys/bio.h>
#include <sys/blist.h>		#include <sys/blist.h>
#include <sys/buf.h>		#include <sys/buf.h>
#include <sys/conf.h>		#include <sys/conf.h>
#include <sys/disk.h>		#include <sys/disk.h>
		#include <sys/disklabel.h>
#include <sys/eventhandler.h>		#include <sys/eventhandler.h>
#include <sys/fcntl.h>		#include <sys/fcntl.h>
#include <sys/lock.h>		#include <sys/lock.h>
#include <sys/kernel.h>		#include <sys/kernel.h>
#include <sys/mount.h>		#include <sys/mount.h>
#include <sys/namei.h>		#include <sys/namei.h>
#include <sys/malloc.h>		#include <sys/malloc.h>
#include <sys/pctrie.h>		#include <sys/pctrie.h>
▲ Show 20 Lines • Show All 2,203 Lines • ▼ Show 20 Lines	swaponsomething(struct vnode vp, void id, u_long nblks,
sp->sw_nblks = nblks;		sp->sw_nblks = nblks;
sp->sw_used = 0;		sp->sw_used = 0;
sp->sw_strategy = strategy;		sp->sw_strategy = strategy;
sp->sw_close = close;		sp->sw_close = close;
sp->sw_flags = flags;		sp->sw_flags = flags;

sp->sw_blist = blist_create(nblks, M_WAITOK);		sp->sw_blist = blist_create(nblks, M_WAITOK);
/*		/*
* Do not free the first two block in order to avoid overwriting		* Do not free the first blocks in order to avoid overwriting
* any bsd label at the front of the partition		* any bsd label at the front of the partition
*/		*/
blist_free(sp->sw_blist, 2, nblks - 2);		blist_free(sp->sw_blist, howmany(BBSIZE, PAGE_SIZE),
		nblks - howmany(BBSIZE, PAGE_SIZE));

		alcUnsubmitted Not Done Inline Actions Consider the possibility that PAGE_SIZE is greater than BBSIZE. (We still barely support one architecture, sparc64, where they are equal.) I think that howmany(BBSIZE, PAGE_SIZE) will "do the right thing." alc: Consider the possibility that PAGE_SIZE is greater than BBSIZE. (We still barely support one…
dvbase = 0;		dvbase = 0;
mtx_lock(&sw_dev_mtx);		mtx_lock(&sw_dev_mtx);
TAILQ_FOREACH(tsp, &swtailq, sw_list) {		TAILQ_FOREACH(tsp, &swtailq, sw_list) {
if (tsp->sw_end >= dvbase) {		if (tsp->sw_end >= dvbase) {
/*		/*
* We put one uncovered page between the devices		* We put one uncovered page between the devices
* in order to definitively prevent any cross-device		* in order to definitively prevent any cross-device
* I/O requests		* I/O requests
*/		*/
dvbase = tsp->sw_end + 1;		dvbase = tsp->sw_end + 1;
}		}
}		}
sp->sw_first = dvbase;		sp->sw_first = dvbase;
sp->sw_end = dvbase + nblks;		sp->sw_end = dvbase + nblks;
TAILQ_INSERT_TAIL(&swtailq, sp, sw_list);		TAILQ_INSERT_TAIL(&swtailq, sp, sw_list);
nswapdev++;		nswapdev++;
swap_pager_avail += nblks - 2;		swap_pager_avail += nblks - howmany(BBSIZE, PAGE_SIZE);
		alcUnsubmitted Not Done Inline Actions This should also use howmany(BBSIZE, PAGE_SIZE). alc: This should also use howmany(BBSIZE, PAGE_SIZE).
swap_total += nblks;		swap_total += nblks;
swapon_check_swzone();		swapon_check_swzone();
swp_sizecheck();		swp_sizecheck();
mtx_unlock(&sw_dev_mtx);		mtx_unlock(&sw_dev_mtx);
EVENTHANDLER_INVOKE(swapon, sp);		EVENTHANDLER_INVOKE(swapon, sp);
}		}

/*		/*
▲ Show 20 Lines • Show All 661 Lines • Show Last 20 Lines