[Maria-developers] [patch 00/11] A number of fixes for buildbot-found problems
Hi Monty, Sanja, all, I finally managed to get a valgrind test run to pass. I needed 11 patches to get there ... It would be really good to get a review of these so I can push, they are needed to get the Valgrind run to pass in buildbot. I think all of them are ok, but for at least some of them I would really like to get another pair of eyes before pushing as I'm not very familiar with the code they touch. Sanja, could you review first 2 patches? They are of mysql-test-run.pl, one of them is related to the previous mysqld.err fixes. Monty, could you review the remaining 9 patches? They are all fixes for Valgrind warnings. Especially the last 4 I am not 100% sure of are the best possible fix, I think you can help. Serg, I checked up on your comment on my earlier dbug.c patch (thanks!), found the problem you described and I think it is fixed in this version of the patch. Monty, I will commit as one commit before pushing, but due to large number of independent changes I found it convenient to split it up using quilt. - Kristian.
My previous fix for not ignoring warnings during server shutdown would terminate the test suite completely in case of extra warnings, omitting the error summary at the end. This patch provides a much nicer summary report which includes all test sequences that had warnings during shutdown. Also add a $fail flag to make 100% sure mysql-test-run.pl will return failure for any kind of problem occured during the test suite. Also revert wrong "internal error" message from earlier patch. --- mysql-test/lib/mtr_report.pm | 21 ++++++++++++++++++++- mysql-test/mysql-test-run.pl | 27 ++++++++++++++------------- 2 files changed, 34 insertions(+), 14 deletions(-) Index: work-5.1-buildbot/mysql-test/lib/mtr_report.pm =================================================================== --- work-5.1-buildbot.orig/mysql-test/lib/mtr_report.pm 2009-04-07 13:40:41.000000000 +0200 +++ work-5.1-buildbot/mysql-test/lib/mtr_report.pm 2009-04-07 13:40:56.000000000 +0200 @@ -187,8 +187,10 @@ sub mtr_report_test ($) { } -sub mtr_report_stats ($) { +sub mtr_report_stats ($$$) { + my $fail= shift; my $tests= shift; + my $extra_warnings= shift; # ---------------------------------------------------------------------- # Find out how we where doing @@ -325,10 +327,27 @@ sub mtr_report_stats ($) { print "All $tot_tests tests were successful.\n\n"; } + if (@$extra_warnings) + { + print <<MSG; +Errors/warnings were found in logfiles during server shutdown after running the +following sequence(s) of tests: +MSG + print " $_\n" for @$extra_warnings; + } + if ( $tot_failed != 0 || $found_problems) { mtr_error("there were failing test cases"); } + elsif (@$extra_warnings) + { + mtr_error("There were errors/warnings in server logs after running test cases."); + } + elsif ($fail) + { + mtr_error("Test suite failure, see messages above for possible cause(s)."); + } } Index: work-5.1-buildbot/mysql-test/mysql-test-run.pl =================================================================== --- work-5.1-buildbot.orig/mysql-test/mysql-test-run.pl 2009-04-07 13:40:41.000000000 +0200 +++ work-5.1-buildbot/mysql-test/mysql-test-run.pl 2009-04-07 13:41:53.000000000 +0200 @@ -357,7 +357,8 @@ sub main { mtr_print_thick_line(); mtr_print_header(); - my ($completed, $fail)= run_test_server($server, $tests, $opt_parallel); + my ($fail, $completed, $extra_warnings)= + run_test_server($server, $tests, $opt_parallel); # Send Ctrl-C to any children still running kill("INT", keys(%children)); @@ -393,10 +394,6 @@ sub main { mtr_error("Not all tests completed"); } - if ($fail) { - mtr_error("Test suite failure."); - } - mtr_print_line(); if ( $opt_gcov ) { @@ -404,7 +401,7 @@ sub main { $opt_gcov_msg, $opt_gcov_err); } - mtr_report_stats($completed); + mtr_report_stats($fail, $completed, $extra_warnings); exit(0); } @@ -416,7 +413,8 @@ sub run_test_server ($$$) { my $num_saved_cores= 0; # Number of core files saved in vardir/log/ so far. my $num_saved_datadir= 0; # Number of datadirs saved in vardir/log/ so far. my $num_failed_test= 0; # Number of tests failed so far - my $test_failure= 0; + my $test_failure= 0; # Set true if test suite failed + my $extra_warnings= []; # Warnings found during server shutdowns # Scheduler variables my $max_ndb= $childs / 2; @@ -450,7 +448,7 @@ sub run_test_server ($$$) { $s->remove($sock); if (--$childs == 0){ $suite_timeout_proc->kill(); - return ($completed, $test_failure); + return ($test_failure, $completed, $extra_warnings); } next; } @@ -519,14 +517,14 @@ sub run_test_server ($$$) { # Test has failed, force is off $suite_timeout_proc->kill(); push(@$completed, $result); - return ($completed, 1); + return (1, $completed, $extra_warnings); } elsif ($opt_max_test_fail > 0 and $num_failed_test >= $opt_max_test_fail) { $suite_timeout_proc->kill(); mtr_report("Too many tests($num_failed_test) failed!", "Terminating..."); - return (undef, 1); + return (1, $completed, $extra_warnings); } $num_failed_test++; } @@ -580,13 +578,14 @@ sub run_test_server ($$$) { elsif ($line eq 'WARNINGS'){ my $fake_test= My::Test::read_test($sock); my $test_list= join (" ", @{$fake_test->{testnames}}); + push @$extra_warnings, $test_list; mtr_report("***Warnings generated in error logs during shutdown ". "after running tests: $test_list"); $test_failure= 1; if ( !$opt_force ) { # Test failure due to warnings, force is off $suite_timeout_proc->kill(); - return ($completed, 1); + return (1, $completed, $extra_warnings); } } else { mtr_error("Unknown response: '$line' from client"); @@ -666,7 +665,7 @@ sub run_test_server ($$$) { if ( ! $suite_timeout_proc->wait_one(0) ) { mtr_report("Test suite timeout! Terminating..."); - return (undef, 1); + return (1, $completed, $extra_warnings); } } } @@ -758,7 +757,9 @@ sub run_worker ($) { } } - die "Internal error: should not reach this place."; + stop_all_servers(); + + exit(1); } --
Hi!
"knielsen" == knielsen <knielsen@knielsen-hq.org> writes:
knielsen> My previous fix for not ignoring warnings during server shutdown would knielsen> terminate the test suite completely in case of extra warnings, knielsen> omitting the error summary at the end. knielsen> This patch provides a much nicer summary report which includes all knielsen> test sequences that had warnings during shutdown. knielsen> Also add a $fail flag to make 100% sure mysql-test-run.pl will return knielsen> failure for any kind of problem occured during the test suite. knielsen> Also revert wrong "internal error" message from earlier patch. <cut> knielsen> -sub mtr_report_stats ($) { knielsen> +sub mtr_report_stats ($$$) { knielsen> + my $fail= shift; knielsen> my $tests= shift; knielsen> + my $extra_warnings= shift; knielsen> knielsen> # ---------------------------------------------------------------------- knielsen> # Find out how we where doing knielsen> @@ -325,10 +327,27 @@ sub mtr_report_stats ($) { knielsen> print "All $tot_tests tests were successful.\n\n"; knielsen> } knielsen> knielsen> + if (@$extra_warnings) knielsen> + { knielsen> + print <<MSG; knielsen> +Errors/warnings were found in logfiles during server shutdown after running the knielsen> +following sequence(s) of tests: knielsen> +MSG knielsen> + print " $_\n" for @$extra_warnings; knielsen> + } knielsen> + knielsen> if ( $tot_failed != 0 || $found_problems) knielsen> { knielsen> mtr_error("there were failing test cases"); knielsen> } knielsen> + elsif (@$extra_warnings) knielsen> + { knielsen> + mtr_error("There were errors/warnings in server logs after running test cases."); knielsen> + } knielsen> + elsif ($fail) knielsen> + { knielsen> + mtr_error("Test suite failure, see messages above for possible cause(s)."); knielsen> + } knielsen> } Looked through the patch, and found only one possible thing to comment about: It lookes liked this case is handled, but from the patch/current code it was not trivial to be sure. - We run some tests, one test fails. - Test suite fails totally (for example timeout) for test 2 I hope that in this case we get a clear error that not all tests are run (as the @tests above probably only includes the test that we did run) ok to push, assuming the above is true. Regards, Monty
Michael Widenius <monty@askmonty.org> writes:
Looked through the patch, and found only one possible thing to comment about:
It lookes liked this case is handled, but from the patch/current code it was not trivial to be sure.
- We run some tests, one test fails. - Test suite fails totally (for example timeout) for test 2
I hope that in this case we get a clear error that not all tests are run (as the @tests above probably only includes the test that we did run)
Yes, this should be handled by the following code, which is run just before the final reporting about failed tests: if ( @$completed != $num_tests){ # Not all tests completed, failure mtr_report(); mtr_report("Only ", int(@$completed), " of $num_tests completed."); mtr_error("Not all tests completed"); } Good point though.
ok to push, assuming the above is true.
Thanks. - Kristian.
The globally writable lock file for the mysql-test-run.pl port allocation was not always made globally writable, causing failures for subsequent runs by other users on the same machine. === modified file 'mysql-test/lib/mtr_unique.pm' --- mysql-test/lib/mtr_unique.pm | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) Index: mysql-test/lib/mtr_unique.pm =================================================================== --- mysql-test/lib/mtr_unique.pm.orig 2009-04-07 13:40:36.000000000 +0200 +++ mysql-test/lib/mtr_unique.pm 2009-04-07 13:42:05.000000000 +0200 @@ -62,13 +62,14 @@ sub mtr_get_unique_id($$) { die 'lock file is a symbolic link'; } - chmod 0777, "$file.sem"; open SEM, ">", "$file.sem" or die "can't write to $file.sem"; + chmod 0777, "$file.sem"; flock SEM, LOCK_EX or die "can't lock $file.sem"; if(! -e $file) { open FILE, ">", $file or die "can't create $file"; close FILE; } + chmod 0777, $file; msg("HAVE THE LOCK"); @@ -76,7 +77,6 @@ sub mtr_get_unique_id($$) { die 'lock file is a symbolic link'; } - chmod 0777, $file; open FILE, "+<", $file or die "can't open $file"; #select undef,undef,undef,0.2; seek FILE, 0, 0; @@ -136,6 +136,7 @@ sub mtr_release_unique_id($) { } open SEM, ">", "$file.sem" or die "can't write to $file.sem"; + chmod 0777, "$file.sem"; flock SEM, LOCK_EX or die "can't lock $file.sem"; msg("HAVE THE LOCK"); @@ -148,6 +149,7 @@ sub mtr_release_unique_id($) { open FILE, ">", $file or die "can't create $file"; close FILE; } + chmod 0777, "$file.sem"; open FILE, "+<", $file or die "can't open $file"; #select undef,undef,undef,0.2; seek FILE, 0, 0; --
Hi!
"knielsen" == knielsen <knielsen@knielsen-hq.org> writes:
knielsen> The globally writable lock file for the mysql-test-run.pl port knielsen> allocation was not always made globally writable, causing failures for knielsen> subsequent runs by other users on the same machine. knielsen> === modified file 'mysql-test/lib/mtr_unique.pm' knielsen> --- knielsen> mysql-test/lib/mtr_unique.pm | 6 ++++-- knielsen> 1 file changed, 4 insertions(+), 2 deletions(-) knielsen> Index: mysql-test/lib/mtr_unique.pm knielsen> =================================================================== knielsen> --- mysql-test/lib/mtr_unique.pm.orig 2009-04-07 13:40:36.000000000 +0200 knielsen> +++ mysql-test/lib/mtr_unique.pm 2009-04-07 13:42:05.000000000 +0200 knielsen> @@ -62,13 +62,14 @@ sub mtr_get_unique_id($$) { knielsen> die 'lock file is a symbolic link'; knielsen> } knielsen> - chmod 0777, "$file.sem"; knielsen> open SEM, ">", "$file.sem" or die "can't write to $file.sem"; knielsen> + chmod 0777, "$file.sem"; knielsen> flock SEM, LOCK_EX or die "can't lock $file.sem"; knielsen> if(! -e $file) { knielsen> open FILE, ">", $file or die "can't create $file"; knielsen> close FILE; knielsen> } knielsen> + chmod 0777, $file; Would it be easier/safer to temporally set the umask for the process, instead of doing a chmod? That way, we don't get a problem if the process dies between open and chmod. That's also a way to avoid the problem that files in the 'var' directory from the previous run can't be deleted by next user. <cut> ok to push, but think about the above idea. Regards, Monty
=== modified file 'sql/mysqld.cc' --- sql/mysqld.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Index: sql/mysqld.cc =================================================================== --- sql/mysqld.cc.orig 2009-04-07 13:40:36.000000000 +0200 +++ sql/mysqld.cc 2009-04-07 13:42:09.000000000 +0200 @@ -4815,10 +4815,10 @@ static bool read_init_file(char *file_na DBUG_ENTER("read_init_file"); DBUG_PRINT("enter",("name: %s",file_name)); if (!(file=my_fopen(file_name,O_RDONLY,MYF(MY_WME)))) - return(1); + DBUG_RETURN(1); bootstrap(file); (void) my_fclose(file,MYF(MY_WME)); - return 0; + DBUG_RETURN(0); } --
Hi!
"knielsen" == knielsen <knielsen@knielsen-hq.org> writes:
knielsen> === modified file 'sql/mysqld.cc' knielsen> --- knielsen> sql/mysqld.cc | 4 ++-- knielsen> 1 file changed, 2 insertions(+), 2 deletions(-) knielsen> Index: sql/mysqld.cc knielsen> =================================================================== knielsen> --- sql/mysqld.cc.orig 2009-04-07 13:40:36.000000000 +0200 knielsen> +++ sql/mysqld.cc 2009-04-07 13:42:09.000000000 +0200 knielsen> @@ -4815,10 +4815,10 @@ static bool read_init_file(char *file_na knielsen> DBUG_ENTER("read_init_file"); knielsen> DBUG_PRINT("enter",("name: %s",file_name)); knielsen> if (!(file=my_fopen(file_name,O_RDONLY,MYF(MY_WME)))) knielsen> - return(1); knielsen> + DBUG_RETURN(1); knielsen> bootstrap(file); knielsen> (void) my_fclose(file,MYF(MY_WME)); knielsen> - return 0; knielsen> + DBUG_RETURN(0); knielsen> } Trivial fix; Always ok to push things like this (after a test compile) Regards, Monty
(Some versions of GCC implement struct_var = struct_var using memcpy(), which violates specs for memcpy() (not allowed for overlapping source and destination). GCC bug 19410: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19410 === modified file 'sql/sql_select.cc' --- sql/sql_select.cc | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) Index: sql/sql_select.cc =================================================================== --- sql/sql_select.cc.orig 2009-04-07 13:40:36.000000000 +0200 +++ sql/sql_select.cc 2009-04-07 13:42:10.000000000 +0200 @@ -1994,8 +1994,17 @@ JOIN::exec() tmp_fields_list2, tmp_all_fields2, fields_list.elements, tmp_all_fields1)) DBUG_VOID_RETURN; - curr_join->tmp_fields_list2= tmp_fields_list2; - curr_join->tmp_all_fields2= tmp_all_fields2; +#ifdef HAVE_purify + /* + Some GCCs use memcpy() for struct assignment, even for x=x. + GCC bug 19410: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19410 + */ + if (curr_join != this) +#endif + { + curr_join->tmp_fields_list2= tmp_fields_list2; + curr_join->tmp_all_fields2= tmp_all_fields2; + } } curr_fields_list= &curr_join->tmp_fields_list2; curr_all_fields= &curr_join->tmp_all_fields2; --
Hi!
"knielsen" == knielsen <knielsen@knielsen-hq.org> writes:
knielsen> (Some versions of GCC implement struct_var = struct_var using knielsen> memcpy(), which violates specs for memcpy() (not allowed for knielsen> overlapping source and destination). knielsen> GCC bug 19410: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19410 knielsen> === modified file 'sql/sql_select.cc' knielsen> --- knielsen> sql/sql_select.cc | 13 +++++++++++-- knielsen> 1 file changed, 11 insertions(+), 2 deletions(-) knielsen> Index: sql/sql_select.cc knielsen> =================================================================== knielsen> --- sql/sql_select.cc.orig 2009-04-07 13:40:36.000000000 +0200 knielsen> +++ sql/sql_select.cc 2009-04-07 13:42:10.000000000 +0200 knielsen> @@ -1994,8 +1994,17 @@ JOIN::exec() knielsen> tmp_fields_list2, tmp_all_fields2, knielsen> fields_list.elements, tmp_all_fields1)) knielsen> DBUG_VOID_RETURN; knielsen> - curr_join->tmp_fields_list2= tmp_fields_list2; knielsen> - curr_join->tmp_all_fields2= tmp_all_fields2; knielsen> +#ifdef HAVE_purify knielsen> + /* knielsen> + Some GCCs use memcpy() for struct assignment, even for x=x. knielsen> + GCC bug 19410: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19410 knielsen> + */ knielsen> + if (curr_join != this) knielsen> +#endif knielsen> + { knielsen> + curr_join->tmp_fields_list2= tmp_fields_list2; knielsen> + curr_join->tmp_all_fields2= tmp_all_fields2; knielsen> + } knielsen> } knielsen> curr_fields_list= &curr_join->tmp_fields_list2; knielsen> curr_all_fields= &curr_join->tmp_all_fields2; ok to push. I think this is actually something that should be fixed in valgrind; Copying over itself shouldn't give a warning. (Or at least, one should be able to disable it) Regards, Monty
(different system library versions generate slightly different stack traces to be suppressed). === modified file 'mysql-test/valgrind.supp' --- mysql-test/valgrind.supp | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) Index: mysql-test/valgrind.supp =================================================================== --- mysql-test/valgrind.supp.orig 2009-04-07 13:40:36.000000000 +0200 +++ mysql-test/valgrind.supp 2009-04-07 13:42:12.000000000 +0200 @@ -379,7 +379,7 @@ } { - dlclose memory loss from plugin + dlclose memory loss from plugin variant 1 Memcheck:Leak fun:calloc fun:_dlerror_run @@ -388,6 +388,19 @@ } { + dlclose memory loss from plugin variant 2 + Memcheck:Leak + fun:malloc + fun:_dl_close_worker + fun:_dl_close + fun:_dl_catch_error + fun:_dlerror_run + fun:dlclose + fun:_Z15free_plugin_memP12st_plugin_dl + fun:_Z13plugin_dl_delPK19st_mysql_lex_string +} + +{ dlopen / ptread_cancel_init memory loss on Suse Linux 10.3 32/64 bit Memcheck:Leak fun:*alloc --
Hi!
"knielsen" == knielsen <knielsen@knielsen-hq.org> writes:
knielsen> (different system library versions generate slightly different stack knielsen> traces to be suppressed). knielsen> === modified file 'mysql-test/valgrind.supp' knielsen> --- knielsen> mysql-test/valgrind.supp | 15 ++++++++++++++- knielsen> 1 file changed, 14 insertions(+), 1 deletion(-) knielsen> Index: mysql-test/valgrind.supp knielsen> =================================================================== knielsen> --- mysql-test/valgrind.supp.orig 2009-04-07 13:40:36.000000000 +0200 knielsen> +++ mysql-test/valgrind.supp 2009-04-07 13:42:12.000000000 +0200 knielsen> @@ -379,7 +379,7 @@ knielsen> } knielsen> { knielsen> - dlclose memory loss from plugin knielsen> + dlclose memory loss from plugin variant 1 knielsen> Memcheck:Leak knielsen> fun:calloc knielsen> fun:_dlerror_run knielsen> @@ -388,6 +388,19 @@ knielsen> } knielsen> { knielsen> + dlclose memory loss from plugin variant 2 knielsen> + Memcheck:Leak knielsen> + fun:malloc knielsen> + fun:_dl_close_worker knielsen> + fun:_dl_close knielsen> + fun:_dl_catch_error knielsen> + fun:_dlerror_run knielsen> + fun:dlclose knielsen> + fun:_Z15free_plugin_memP12st_plugin_dl knielsen> + fun:_Z13plugin_dl_delPK19st_mysql_lex_string knielsen> +} knielsen> + knielsen> +{ knielsen> dlopen / ptread_cancel_init memory loss on Suse Linux 10.3 32/64 bit knielsen> Memcheck:Leak knielsen> fun:*alloc ok to push. Regards, Monty
=== modified file 'mysql-test/valgrind.supp' --- mysql-test/valgrind.supp | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) Index: mysql-test/valgrind.supp =================================================================== --- mysql-test/valgrind.supp.orig 2009-04-07 13:42:12.000000000 +0200 +++ mysql-test/valgrind.supp 2009-04-07 13:42:14.000000000 +0200 @@ -601,3 +601,19 @@ fun:dlopen* } +# +# In glibc (checked version 2.7), inet_ntoa allocates an 18-byte +# per-thread static buffer for the return value. That memory is freed +# at thread exit, however if called from the main thread, Valgrind +# does not see the free (test main.no-threads). +# +# Since inet_ntoa() does not allocate memory dynamically per-call, this +# suppression is safe. +# + +{ + inet_ntoa thread local storage + Memcheck:Leak + fun:malloc + fun:inet_ntoa +} --
Hi!
"knielsen" == knielsen <knielsen@knielsen-hq.org> writes:
knielsen> === modified file 'mysql-test/valgrind.supp' knielsen> --- knielsen> mysql-test/valgrind.supp | 16 ++++++++++++++++ knielsen> 1 file changed, 16 insertions(+) knielsen> Index: mysql-test/valgrind.supp knielsen> =================================================================== knielsen> --- mysql-test/valgrind.supp.orig 2009-04-07 13:42:12.000000000 +0200 knielsen> +++ mysql-test/valgrind.supp 2009-04-07 13:42:14.000000000 +0200 knielsen> @@ -601,3 +601,19 @@ knielsen> fun:dlopen* knielsen> } knielsen> +# knielsen> +# In glibc (checked version 2.7), inet_ntoa allocates an 18-byte knielsen> +# per-thread static buffer for the return value. That memory is freed knielsen> +# at thread exit, however if called from the main thread, Valgrind knielsen> +# does not see the free (test main.no-threads). knielsen> +# knielsen> +# Since inet_ntoa() does not allocate memory dynamically per-call, this knielsen> +# suppression is safe. knielsen> +# knielsen> + knielsen> +{ knielsen> + inet_ntoa thread local storage knielsen> + Memcheck:Leak knielsen> + fun:malloc knielsen> + fun:inet_ntoa knielsen> +} Ok (Good comment) Regards, Monty
The zlib code does operations on uninitialised parts of internal memory, but subsequently does bounds checks and ignores the results of any such undefined operations. This causes Valgrind to give false alarms. Fix by making memory allocated by zlib initialised #ifdef HAVE_purify. Also make zlib use my_malloc() for better leak checking etc. This is better than adding lots of suppressions for zlib operations: - No need to constantly add new suppressions for different versions of zlib. - Less risk of suppressing a real problem (like trying to compress/decompress uninitialised data). === modified file 'storage/archive/azio.c' --- storage/archive/azio.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 42 insertions(+), 2 deletions(-) Index: storage/archive/azio.c =================================================================== --- storage/archive/azio.c.orig 2009-04-07 13:40:36.000000000 +0200 +++ storage/archive/azio.c 2009-04-07 13:42:16.000000000 +0200 @@ -37,6 +37,46 @@ void putLong(File file, uLong x); uLong getLong(azio_stream *s); void read_header(azio_stream *s, unsigned char *buffer); +/* + Valgrind normally gives false alarms for zlib operations, in the form of + "conditional jump depends on uninitialised values" etc. The reason is + explained in the zlib FAQ (http://www.zlib.net/zlib_faq.html#faq36): + + "That is intentional for performance reasons, and the output of deflate + is not affected." + + Also discussed on a blog + (http://www.sirena.org.uk/log/2006/02/19/zlib-generating-valgrind-warnings/): + + "...loop unrolling in the zlib library causes the mentioned + “Conditional jump or move depends on uninitialised value(s)” + warnings. These are safe since the results of the comparison are + subsequently ignored..." + + "the results of the calculations are discarded by bounds checking done + after the loop exits" + + Fix by initializing the memory allocated by zlib when running under Valgrind. + + This fix is safe, since such memory is only used internally by zlib, so we + will not hide any bugs in mysql this way. +*/ +static void *az_allocator(void *dummy, uInt items, uInt size) +{ + return my_malloc((size_t)items * (size_t)size, +#ifdef HAVE_purify + MY_ZEROFILL +#else + MYF(0) +#endif + ); +} + +static void az_free(void *dummy, void *address) +{ + my_free(address, MYF(MY_ALLOW_ZERO_PTR)); +} + /* =========================================================================== Opens a gzip (.gz) file for reading or writing. The mode parameter is as in fopen ("rb" or "wb"). The file is given either by file descriptor @@ -52,8 +92,8 @@ int az_open (azio_stream *s, const char int level = Z_DEFAULT_COMPRESSION; /* compression level */ int strategy = Z_DEFAULT_STRATEGY; /* compression strategy */ - s->stream.zalloc = (alloc_func)0; - s->stream.zfree = (free_func)0; + s->stream.zalloc = az_allocator; + s->stream.zfree = az_free; s->stream.opaque = (voidpf)0; memset(s->inbuf, 0, AZ_BUFSIZE_READ); memset(s->outbuf, 0, AZ_BUFSIZE_WRITE); --
Hi!
"knielsen" == knielsen <knielsen@knielsen-hq.org> writes:
knielsen> The zlib code does operations on uninitialised parts of internal knielsen> memory, but subsequently does bounds checks and ignores the results of knielsen> any such undefined operations. This causes Valgrind to give false knielsen> alarms. <cut> knielsen> +static void *az_allocator(void *dummy, uInt items, uInt size) knielsen> +{ knielsen> + return my_malloc((size_t)items * (size_t)size, knielsen> +#ifdef HAVE_purify knielsen> + MY_ZEROFILL knielsen> +#else knielsen> + MYF(0) knielsen> +#endif knielsen> + ); We may want to do things like this in other places to. I suggest you add to my_global.h: /* Make it easier to add conditionl code for valgrind/purify */ #ifdef HAVE_purify #define IF_PURIFY(A,B) (A) #else #define IF_PURIFY(A,B) (B) #endif And then instead use above: return my_malloc((size_t)items * (size_t)size, IF_PURIFY(MY_ZEROFILL, 0)); As a separate note, we should consider doing a replacement in the code: PURIFY -> VALGRIND purify -> valgrind No reason to promote a commerical tool we don't use anymore... Regards, Monty
After implementing pool_of_threads, the thd->connect_utime and thd->start_utime are not always correctly initialized (depending on which thread model is used). === modified file 'sql/mysqld.cc' --- sql/mysqld.cc | 1 + sql/scheduler.cc | 1 + 2 files changed, 2 insertions(+) Index: sql/mysqld.cc =================================================================== --- sql/mysqld.cc.orig 2009-04-07 13:42:09.000000000 +0200 +++ sql/mysqld.cc 2009-04-07 13:42:18.000000000 +0200 @@ -4839,6 +4839,7 @@ void handle_connection_in_main_thread(TH safe_mutex_assert_owner(&LOCK_thread_count); thread_cache_size=0; // Safety threads.append(thd); + thd->connect_utime= thd->start_utime= my_micro_time(); (void) pthread_mutex_unlock(&LOCK_thread_count); handle_one_connection((void*) thd); } Index: sql/scheduler.cc =================================================================== --- sql/scheduler.cc.orig 2009-04-07 13:40:35.000000000 +0200 +++ sql/scheduler.cc 2009-04-07 13:42:18.000000000 +0200 @@ -470,6 +470,7 @@ static void libevent_add_connection(THD DBUG_VOID_RETURN; } threads.append(thd); + thd->connect_utime= thd->start_utime= my_micro_time(); libevent_thd_add(thd); pthread_mutex_unlock(&LOCK_thread_count); --
Hi!
"knielsen" == knielsen <knielsen@knielsen-hq.org> writes:
knielsen> After implementing pool_of_threads, the thd->connect_utime and thd-> start_utime are not always correctly initialized (depending on knielsen> which thread model is used). knielsen> === modified file 'sql/mysqld.cc' knielsen> --- knielsen> sql/mysqld.cc | 1 + knielsen> sql/scheduler.cc | 1 + knielsen> 2 files changed, 2 insertions(+) knielsen> Index: sql/mysqld.cc knielsen> =================================================================== knielsen> --- sql/mysqld.cc.orig 2009-04-07 13:42:09.000000000 +0200 knielsen> +++ sql/mysqld.cc 2009-04-07 13:42:18.000000000 +0200 knielsen> @@ -4839,6 +4839,7 @@ void handle_connection_in_main_thread(TH knielsen> safe_mutex_assert_owner(&LOCK_thread_count); knielsen> thread_cache_size=0; // Safety knielsen> threads.append(thd); knielsen> + thd->connect_utime= thd->start_utime= my_micro_time(); Couldn't find any way how connect_utime could be used uninitialized. This is only used in handle_one_connection(), to calculate the time it took to start a thread. start_utime is initialized in THD::THD() to 0 and to my_micro_time() at the start of a query. In which case was it used wrongly? knielsen> (void) pthread_mutex_unlock(&LOCK_thread_count); knielsen> handle_one_connection((void*) thd); knielsen> } knielsen> Index: sql/scheduler.cc knielsen> =================================================================== knielsen> --- sql/scheduler.cc.orig 2009-04-07 13:40:35.000000000 +0200 knielsen> +++ sql/scheduler.cc 2009-04-07 13:42:18.000000000 +0200 knielsen> @@ -470,6 +470,7 @@ static void libevent_add_connection(THD knielsen> DBUG_VOID_RETURN; knielsen> } knielsen> threads.append(thd); knielsen> + thd->connect_utime= thd->start_utime= my_micro_time(); knielsen> libevent_thd_add(thd); Same questions as for above. Regards, Monty
If DbugParse() is called multiple times, the stack->keywords for the top stack frame could be overwritten without being freed, causing a memory leak reported by Valgrind. === modified file 'dbug/dbug.c' --- dbug/dbug.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) Index: dbug/dbug.c =================================================================== --- dbug/dbug.c.orig 2009-04-07 13:40:35.000000000 +0200 +++ dbug/dbug.c 2009-04-08 00:35:07.000000000 +0200 @@ -506,6 +506,9 @@ int DbugParse(CODE_STATE *cs, const char rel= control[0] == '+' || control[0] == '-'; if ((!rel || (!stack->out_file && !stack->next))) { + /* If overwriting previous state, be sure to free old to avoid leak. */ + if (stack->out_file) + FreeState(cs, stack, 0); stack->flags= 0; stack->delay= 0; stack->maxdepth= 0; @@ -1648,10 +1651,12 @@ static void FreeState(CODE_STATE *cs, st FreeList(state->processes); if (!is_shared(state, p_functions)) FreeList(state->p_functions); - if (!is_shared(state, out_file)) + if (!is_shared(state, out_file) && + state->out_file != stderr && state->out_file != stdout) DBUGCloseFile(cs, state->out_file); (void) fflush(cs->stack->out_file); - if (state->prof_file) + if (state->prof_file && + state->prof_file != stderr && state->prof_file != stdout) DBUGCloseFile(cs, state->prof_file); if (free_state) free((void*) state); --
hi!
"knielsen" == knielsen <knielsen@knielsen-hq.org> writes:
knielsen> If DbugParse() is called multiple times, the stack->keywords for the knielsen> top stack frame could be overwritten without being freed, causing a knielsen> memory leak reported by Valgrind. knielsen> === modified file 'dbug/dbug.c' knielsen> --- knielsen> dbug/dbug.c | 9 +++++++-- knielsen> 1 file changed, 7 insertions(+), 2 deletions(-) <cut> ok to push. Regards, Monty
The sql_string.h String class has some strange code to null-terminate the string for String::c_ptr(). It checks if the string is already null-terminated, and only writes the '\0' at the end if it is not already there. The reason for this I think is to avoid the re-allocation of the buffer to fit the null termination at append time if it is not needed due to never calling c_ptr(). However, the problem case here is a different one, where the null termination fits in the given buffer, but the value is uninitialised. In this case, checking before zeroing works (if in a somewhat twisted way). But it causes a false Valgrind alarm. This is a minimal fix; not sure if it is the right one. Maybe it would be better to fix the c_str() method to not use this tricky way of working? --- sql/sql_string.h | 8 ++++++++ 1 file changed, 8 insertions(+) Index: work-5.1-buildbot/sql/sql_string.h =================================================================== --- work-5.1-buildbot.orig/sql/sql_string.h 2009-04-08 00:34:49.000000000 +0200 +++ work-5.1-buildbot/sql/sql_string.h 2009-04-08 00:35:38.000000000 +0200 @@ -99,7 +99,15 @@ public: inline const char *ptr() const { return Ptr; } inline char *c_ptr() { + /* + This code null-terminates the string if not already null-terminated. + This works whether Ptr[str_length] is undefined or not, but if undefined + it generates a Valgrind error. So in the Valgrind case, null-terminate + unconditionally. + */ +#ifndef HAVE_purify if (!Ptr || Ptr[str_length]) /* Should be safe */ +#endif (void) realloc(str_length); return Ptr; } --
Hi!
"knielsen" == knielsen <knielsen@knielsen-hq.org> writes:
knielsen> The sql_string.h String class has some strange code to null-terminate knielsen> the string for String::c_ptr(). knielsen> It checks if the string is already null-terminated, and only writes knielsen> the '\0' at the end if it is not already there. knielsen> The reason for this I think is to avoid the re-allocation of the buffer to fit the null termination at append time if it is not needed due to never calling c_ptr(). Yes. knielsen> However, the problem case here is a different one, where the null knielsen> termination fits in the given buffer, but the value is uninitialised. In what code did this happen ? In most of MySQL code we will put a \0 at end when we generate a string. knielsen> In this case, checking before zeroing works (if in a somewhat twisted knielsen> way). But it causes a false Valgrind alarm. knielsen> This is a minimal fix; not sure if it is the right one. Maybe it would knielsen> be better to fix the c_str() method to not use this tricky way of knielsen> working? The problem with fixing c_ptr() is that we would have to do a lot of unnecessary mallocs; This is in particular true when you use a String as a pointer to a null terminated C string. In this case the end \0 is not part of the string and a c_ptr() would force us to do a not-needed malloc. knielsen> --- knielsen> sql/sql_string.h | 8 ++++++++ knielsen> 1 file changed, 8 insertions(+) knielsen> Index: work-5.1-buildbot/sql/sql_string.h knielsen> =================================================================== knielsen> --- work-5.1-buildbot.orig/sql/sql_string.h 2009-04-08 00:34:49.000000000 +0200 knielsen> +++ work-5.1-buildbot/sql/sql_string.h 2009-04-08 00:35:38.000000000 +0200 knielsen> @@ -99,7 +99,15 @@ public: knielsen> inline const char *ptr() const { return Ptr; } knielsen> inline char *c_ptr() knielsen> { knielsen> + /* knielsen> + This code null-terminates the string if not already null-terminated. knielsen> + This works whether Ptr[str_length] is undefined or not, but if undefined knielsen> + it generates a Valgrind error. So in the Valgrind case, null-terminate knielsen> + unconditionally. knielsen> + */ knielsen> +#ifndef HAVE_purify knielsen> if (!Ptr || Ptr[str_length]) /* Should be safe */ knielsen> +#endif knielsen> (void) realloc(str_length); knielsen> return Ptr; knielsen> } In this case I would like to keep the original code to ensure we don't access unallocated or uninitialized memory. Regards, Monty
In set_var.cc, several methods construct a String object passing too large lenght for given buffer. The String class assumes 1 more byte is available after the given length for zero termination in String::c_ptr(). Fix by passing proper lenght in constructor call. --- sql/set_var.cc | 14 +++++++------- sql/sql_string.h | 4 ++++ 2 files changed, 11 insertions(+), 7 deletions(-) Index: work-5.1-buildbot/sql/set_var.cc =================================================================== --- work-5.1-buildbot.orig/sql/set_var.cc 2009-04-08 00:34:49.000000000 +0200 +++ work-5.1-buildbot/sql/set_var.cc 2009-04-08 00:35:43.000000000 +0200 @@ -1740,7 +1740,7 @@ bool sys_var::check_enum(THD *thd, set_v { char buff[STRING_BUFFER_USUAL_SIZE]; const char *value; - String str(buff, sizeof(buff), system_charset_info), *res; + String str(buff, sizeof(buff) - 1, system_charset_info), *res; if (var->value->result_type() == STRING_RESULT) { @@ -1777,7 +1777,7 @@ bool sys_var::check_set(THD *thd, set_va bool not_used; char buff[STRING_BUFFER_USUAL_SIZE], *error= 0; uint error_len= 0; - String str(buff, sizeof(buff), system_charset_info), *res; + String str(buff, sizeof(buff) - 1, system_charset_info), *res; if (var->value->result_type() == STRING_RESULT) { @@ -1942,7 +1942,7 @@ bool sys_var_thd_date_time_format::updat bool sys_var_thd_date_time_format::check(THD *thd, set_var *var) { char buff[STRING_BUFFER_USUAL_SIZE]; - String str(buff,sizeof(buff), system_charset_info), *res; + String str(buff,sizeof(buff) - 1, system_charset_info), *res; DATE_TIME_FORMAT *format; if (!(res=var->value->val_str(&str))) @@ -2047,7 +2047,7 @@ bool sys_var_collation::check(THD *thd, if (var->value->result_type() == STRING_RESULT) { char buff[STRING_BUFFER_USUAL_SIZE]; - String str(buff,sizeof(buff), system_charset_info), *res; + String str(buff,sizeof(buff) - 1, system_charset_info), *res; if (!(res=var->value->val_str(&str))) { my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), name, "NULL"); @@ -2082,7 +2082,7 @@ bool sys_var_character_set::check(THD *t if (var->value->result_type() == STRING_RESULT) { char buff[STRING_BUFFER_USUAL_SIZE]; - String str(buff,sizeof(buff), system_charset_info), *res; + String str(buff,sizeof(buff) - 1, system_charset_info), *res; if (!(res=var->value->val_str(&str))) { if (!nullable) @@ -3620,7 +3620,7 @@ bool sys_var_thd_storage_engine::check(T { char buff[STRING_BUFFER_USUAL_SIZE]; const char *value; - String str(buff, sizeof(buff), &my_charset_latin1), *res; + String str(buff, sizeof(buff) - 1, &my_charset_latin1), *res; var->save_result.plugin= NULL; if (var->value->result_type() == STRING_RESULT) @@ -3737,7 +3737,7 @@ sys_var_thd_sql_mode:: symbolic_mode_representation(THD *thd, ulonglong val, LEX_STRING *rep) { char buff[STRING_BUFFER_USUAL_SIZE*8]; - String tmp(buff, sizeof(buff), &my_charset_latin1); + String tmp(buff, sizeof(buff) - 1, &my_charset_latin1); tmp.length(0); Index: work-5.1-buildbot/sql/sql_string.h =================================================================== --- work-5.1-buildbot.orig/sql/sql_string.h 2009-04-08 00:35:38.000000000 +0200 +++ work-5.1-buildbot/sql/sql_string.h 2009-04-08 00:35:43.000000000 +0200 @@ -63,6 +63,10 @@ public: Ptr=(char*) str; str_length=(uint) strlen(str); Alloced_length=0; alloced=0; str_charset=cs; } + /* + NOTE: the following two contructors needs the size of memory for STR to be + at least LEN+1 (to make room for zero termination in c_ptr()). + */ String(const char *str,uint32 len, CHARSET_INFO *cs) { Ptr=(char*) str; str_length=len; Alloced_length=0; alloced=0; --
Hi!
"knielsen" == knielsen <knielsen@knielsen-hq.org> writes:
knielsen> In set_var.cc, several methods construct a String object passing too knielsen> large lenght for given buffer. The String class assumes 1 more byte is knielsen> available after the given length for zero termination in knielsen> String::c_ptr(). knielsen> Fix by passing proper lenght in constructor call. <cut> ok above. knielsen> Index: work-5.1-buildbot/sql/sql_string.h knielsen> =================================================================== knielsen> --- work-5.1-buildbot.orig/sql/sql_string.h 2009-04-08 00:35:38.000000000 +0200 knielsen> +++ work-5.1-buildbot/sql/sql_string.h 2009-04-08 00:35:43.000000000 +0200 knielsen> @@ -63,6 +63,10 @@ public: knielsen> Ptr=(char*) str; str_length=(uint) strlen(str); Alloced_length=0; alloced=0; knielsen> str_charset=cs; knielsen> } knielsen> + /* knielsen> + NOTE: the following two contructors needs the size of memory for STR to be knielsen> + at least LEN+1 (to make room for zero termination in c_ptr()). Add: If one intend to use the c_ptr() method. knielsen> + */ knielsen> String(const char *str,uint32 len, CHARSET_INFO *cs) knielsen> { knielsen> Ptr=(char*) str; str_length=len; Alloced_length=0; alloced=0; Regards, Monty
participants (3)
-
knielsen@knielsen-hq.org
-
Kristian Nielsen
-
Michael Widenius