Re: [Bug-wget] [PATCH] Add valgrind testing support via ./configure

[Tim Ruehsen]

On Wednesday 08 October 2014 22:22:12 Darshit Shah wrote:
> On 10/08, Tim Rühsen wrote:
> >Hi,
> >
> >this patch allows for using valgrind memcheck for both test suites via
> >
> >     ./configure --enable-valgrind-tests
> >
> >With valgrind memcheck 24 (perl) tests fail due to lost memory blocks.
> >The python tests all survive (thanks to Darshits thorough testing).
> >
> >Since valgrind is pretty slow, you can of course start a single test with
> >valgrind like
> >
> >     cd tests
> >     export VALGRIND_TESTS="valgrind --error-exitcode=301 --leak-check=full 
> > --
> >
> >track-origins=yes"
> >
> >     ./Test-N-smaller.px
> >
> >This allows also testing with other tools or valgrind options (e.g.
> >valgrind --tool=callgrind for performance tuning).
> >
> >Tim
> 
> Hi Tim,
> 
> I'm not entirely sold on the idea of passing a complete command line and
> executing it through the tests. At the very least its a minor security
> vulnerability for everyone running make check. An exported environment
> variable could then lead to arbitrary code execution.

Only if you already have been hacked. And don't forget, 'making' source code 
that you downloaded from somewhere is a *HUGE* security thread.

But if you are not hacked and trust Wget source code, in what way do you think 
is an exported variable a security thread ? Could you give me an example ?
And even if, what about variables like EDITOR which how a command (e.g. for 
crontab -e) ? It seems pretty standard to have ENV vars with commands in it.

> I cannot generalize this next point, but from my own personal use case,
> valgrind based tests are run when making some changes to the source which
> could potentially lead to memory leaks. In such a case, I would want to run
> a single test under valgrind, and not the complete test suite. Having to
> type the complete valgrind command before executing the test, or exporting
> a relevant environment variable earlier seems like a bad way to do it. I
> would prefer that the command be hard coded into the test suites itself. If
> a more specialized command needs to be run, the user can always run it
> directly:
> valgrind --tool=callgrind ./Test-O.px
> Will still work.

No, it won't. You want to valgrind Wget and not Testt-O.px, which doesn't work 
anyways since it is a script.
With my patch you are able to do that:
VALGRIND_TESTS="valgrind --tool=callgrind" ./Test-O.px

Each developer will organize it in a different way.
My personally approach would be to put 
        VALGRIND_MEMCHECK="valgrind..."
        VALGRIND_CALLGRIND="valgrind --tool=callgrind..."
into .bashrc

And when needed simply type
VALGRIND_TESTS=$VALGRIND_MEMCHECK ./Test-O.px

or if needed more often
export VALGRIND_TESTS=$VALGRIND_MEMCHECK
./Test-O.px
(change code)
./Test-O.px
...
./Test-O.px
(done with valgrind)
unset VALGRIND_TESTS

Such workflow seems pretty common to me.

> Hence, hard coding the command actually reduces the amount of work a user
> needs to do in order to run the tests under valgrind.
> 
> My suggestion is that we allow the configure option, but hard code the
> valgrind command into the test suites themselves, and not leave them
> environment variables.

What about removing the configure option and
if VALGRIND_TESTS is undefined or empty or "0": normal testing
if VALGRIND_TESTS is "1": valgrind testing with hard-coded options
else: testing with command given in VALGRIND_TESTS

The above described workflow should still work, right ?
And we could do the complete test suite with
VALGRIND_TESTS="1" make check

What do you think ?

Tim

signature.asc
Description: This is a digitally signed message part.