[nginx-tests] Add valgrind support to nginx test suite
Maxim Dounin
mdounin at mdounin.ru
Thu May 16 07:45:49 UTC 2024
Hello!
On Mon, May 13, 2024 at 04:43:00PM +1000, Robert Mueller wrote:
> # HG changeset patch
> # User Rob Mueller <robm at fastmailteam.com>
> Add valgrind support to nginx test suite
>
> Setting TEST_NGINX_VALGRIND=1 will cause the test suite
> to run nginx under valgrind and check at exit that there's
> no errors present
> ---
> lib/Test/Nginx.pm | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/lib/Test/Nginx.pm b/lib/Test/Nginx.pm
> index 7b7bdc5..f363349 100644
> --- a/lib/Test/Nginx.pm
> +++ b/lib/Test/Nginx.pm
> @@ -86,7 +86,12 @@ sub DESTROY {
> local $Test::Nginx::TODO;
> my $errors = $self->read_file('error.log');
> $errors = join "\n", $errors =~ /.+Sanitizer.+/gm;
> - Test::More::is($errors, '', 'no sanitizer errors');
> + my $extra = "";
> + if ($ENV{TEST_NGINX_VALGRIND}) {
> + $extra = "/valgrind";
> + $errors .= $self->read_file('valgrind.log');
> + }
> + Test::More::is($errors, '', "no sanitizer${extra} errors");
> }
>
> if ($ENV{TEST_NGINX_CATLOG}) {
> @@ -398,7 +403,11 @@ sub run(;$) {
> my @globals = $self->{_test_globals} ?
> () : ('-g', "pid $testdir/nginx.pid; "
> . "error_log $testdir/error.log debug;");
> - exec($NGINX, '-p', "$testdir/", '-c', 'nginx.conf',
> + my @cmd = ($NGINX);
> + if ($ENV{TEST_NGINX_VALGRIND}) {
> + unshift @cmd, 'valgrind', '-q', "--log-file=$testdir/valgrind.log";
> + }
> + exec(@cmd, '-p', "$testdir/", '-c', 'nginx.conf',
> '-e', 'error.log', @globals)
> or die "Unable to exec(): $!\n";
> }
In my experience, running tests under Valgrind is usually a
challenge, and it rarely make sense now, as compiler-provided
sanitizers are mostly equivalent (and way faster). Further, it
probably can be done via custom TEST_NGINX_BINARY with a wrapper
which runs nginx under Valgrind.
Still, I personally used to patch run() when needed, and
introducing an explicit support for Valgrind might be beneficial
at least for some.
Below are patches which clean up related areas, and introduce an
explicit Valgrind support as a separate destructor test. Note
that Valgrind logging seems to interfere with error suppression in
tests and catches various startup warnings, so errors are filtered
similarly to how it is done with error.log to catch sanitizer
errors. I also used an exec() code which matches existing
approach with adding @globals, and modified waitforfile() timeout
so that tests don't fail due to very slow startup times with
Valgrind.
Also there is a couple of fixes of issues as reported by Valgrind
during full test run, see below.
With these patches, I am able to run all tests with Valgrind
without any real test failures. (Still, some tests occasionally
fail due to Valgrind being very slow.)
Please let me know if it looks good for you.
# HG changeset patch
# User Maxim Dounin <mdounin at mdounin.ru>
# Date 1715843768 -10800
# Thu May 16 10:16:08 2024 +0300
# Node ID 32b470da408173624ad598d4bdc919dac0bbe59f
# Parent 79753dd514e60b47375e36c54739ea434e04a5b6
Tests: avoid changing non-localized $TODO.
This ensures that there will be no unrelated effects if the variable
is actually changed, such as seen on sanitizer tests in 910:49579dd88e3f
(reverted by this change).
diff --git a/lib/Test/Nginx.pm b/lib/Test/Nginx.pm
--- a/lib/Test/Nginx.pm
+++ b/lib/Test/Nginx.pm
@@ -69,21 +69,19 @@ sub DESTROY {
my @alerts = $self->read_file('error.log') =~ /.+\[alert\].+/gm;
- if ($^O eq 'solaris') {
- $Test::Nginx::TODO = 'alerts' if @alerts
- && ! grep { $_ !~ /phantom event/ } @alerts;
- }
- if ($^O eq 'MSWin32') {
- my $re = qr/CloseHandle|TerminateProcess/;
- $Test::Nginx::TODO = 'alerts' if @alerts
- && ! grep { $_ !~ $re } @alerts;
- }
+ local $Test::Nginx::TODO = 'alerts' if @alerts
+ && $^O eq 'solaris'
+ && ! grep { $_ !~ /phantom event/ } @alerts;
+
+ local $Test::Nginx::TODO = 'alerts' if @alerts
+ && $^O eq 'MSWin32'
+ && ! grep { $_ !~ qr/CloseHandle|TerminateProcess/ }
+ @alerts;
Test::More::is(join("\n", @alerts), '', 'no alerts');
}
if (Test::More->builder->expected_tests) {
- local $Test::Nginx::TODO;
my $errors = $self->read_file('error.log');
$errors = join "\n", $errors =~ /.+Sanitizer.+/gm;
Test::More::is($errors, '', 'no sanitizer errors');
# HG changeset patch
# User Maxim Dounin <mdounin at mdounin.ru>
# Date 1715843977 -10800
# Thu May 16 10:19:37 2024 +0300
# Node ID 28670f23380ac08a0121d0a97cdb6a092bb01a31
# Parent 32b470da408173624ad598d4bdc919dac0bbe59f
Tests: explicit Valgrind support.
Valgrind logging is done to a separate file, as it is not able to
follow stderr redirection within nginx or append to a file without
corrupting it. Further, Valgrind logging seems to interfere with
error suppression in tests, and catches various startup errors and
warnings, so the log is additionally filtered.
Since startup under Valgrind can be really slow, timeout in waitforfile()
was changed to 10 seconds.
Prodded by Robert Mueller.
diff --git a/README b/README
--- a/README
+++ b/README
@@ -40,6 +40,10 @@ TEST_NGINX_UNSAFE
Run unsafe tests.
+TEST_NGINX_VALGRIND
+
+ Run nginx under Valgrind during tests.
+
TEST_NGINX_GLOBALS
Sets additional directives in main context.
diff --git a/lib/Test/Nginx.pm b/lib/Test/Nginx.pm
--- a/lib/Test/Nginx.pm
+++ b/lib/Test/Nginx.pm
@@ -87,6 +87,12 @@ sub DESTROY {
Test::More::is($errors, '', 'no sanitizer errors');
}
+ if (Test::More->builder->expected_tests && $ENV{TEST_NGINX_VALGRIND}) {
+ my $errors = $self->read_file('valgrind.log');
+ $errors = join "\n", $errors =~ /^==\d+== .+/gm;
+ Test::More::is($errors, '', 'no valgrind errors');
+ }
+
if ($ENV{TEST_NGINX_CATLOG}) {
system("cat $self->{_testdir}/error.log");
}
@@ -365,7 +371,10 @@ sub try_run($$) {
sub plan($) {
my ($self, $plan) = @_;
- Test::More::plan(tests => $plan + 2);
+ $plan += 2;
+ $plan += 1 if $ENV{TEST_NGINX_VALGRIND};
+
+ Test::More::plan(tests => $plan);
return $self;
}
@@ -395,7 +404,10 @@ sub run(;$) {
my @globals = $self->{_test_globals} ?
() : ('-g', "pid $testdir/nginx.pid; "
. "error_log $testdir/error.log debug;");
- exec($NGINX, '-p', "$testdir/", '-c', 'nginx.conf',
+ my @valgrind = (not $ENV{TEST_NGINX_VALGRIND}) ?
+ () : ('valgrind', '-q',
+ "--log-file=$testdir/valgrind.log");
+ exec(@valgrind, $NGINX, '-p', "$testdir/", '-c', 'nginx.conf',
'-e', 'error.log', @globals)
or die "Unable to exec(): $!\n";
}
@@ -481,7 +493,7 @@ sub waitforfile($;$) {
# wait for file to appear
# or specified process to exit
- for (1 .. 50) {
+ for (1 .. 100) {
return 1 if -e $file;
return 0 if $exited;
$exited = waitpid($pid, WNOHANG) != 0 if $pid;
And here is a patch which fixes issues as reported by Valgrind:
# HG changeset patch
# User Maxim Dounin <mdounin at mdounin.ru>
# Date 1715840681 -10800
# Thu May 16 09:24:41 2024 +0300
# Node ID 7fe30d2ccac58c91b057291ab1eee7949cbd6f69
# Parent 46ecad404a296042c0088e699f275a92758e5ab9
Fixed Valgrind complaints about uninitialized values.
In ngx_http_source_charset(), name->data was left uninitialized, and
only name->len was set. Since it is used in debug logging, this resulted
in the following complaints from Valgrind:
==42== Conditional jump or move depends on uninitialised value(s)
==42== at 0x12BC66: memcpy (string.h:51)
==42== by 0x12BC66: ngx_sprintf_str (ngx_string.c:586)
==42== by 0x12C03C: ngx_vslprintf (ngx_string.c:255)
==42== by 0x127694: ngx_log_error_core (ngx_log.c:135)
==42== by 0x1B8795: ngx_http_charset_header_filter (ngx_http_charset_filter_module.c:252)
Similarly, ngx_http_split_args() returned uninitialized arg->data, which
was then copied to r->args, and also used in debug logging:
==42== Conditional jump or move depends on uninitialised value(s)
==42== at 0x12BC10: memcpy (string.h:50)
==42== by 0x12BC10: ngx_sprintf_str (ngx_string.c:586)
==42== by 0x12C03C: ngx_vslprintf (ngx_string.c:255)
==42== by 0x127694: ngx_log_error_core (ngx_log.c:135)
==42== by 0x184EFB: ngx_http_internal_redirect (ngx_http_core_module.c:2526)
==42== by 0x1D8CCC: ngx_http_try_files_handler (ngx_http_try_files_module.c:209)
Fix is to initialize data to NULL. Note that, while memcpy(p, NULL, 0)
is also formally undefined now, it is used in multiple places in the code,
and expected to be allowed in C2y (see WG14 proposals N3177, N3261,
"Allow zero length operations on null pointers").
Prodded by Valgrind.
diff --git a/src/http/modules/ngx_http_charset_filter_module.c b/src/http/modules/ngx_http_charset_filter_module.c
--- a/src/http/modules/ngx_http_charset_filter_module.c
+++ b/src/http/modules/ngx_http_charset_filter_module.c
@@ -438,6 +438,7 @@ ngx_http_source_charset(ngx_http_request
if (charset == NGX_HTTP_CHARSET_OFF) {
name->len = 0;
+ name->data = NULL;
return charset;
}
diff --git a/src/http/ngx_http_parse.c b/src/http/ngx_http_parse.c
--- a/src/http/ngx_http_parse.c
+++ b/src/http/ngx_http_parse.c
@@ -2146,6 +2146,7 @@ ngx_http_split_args(ngx_http_request_t *
} else {
args->len = 0;
+ args->data = NULL;
}
}
--
Maxim Dounin
http://mdounin.ru/
More information about the nginx-devel
mailing list