[for-2.6?] qemu-iotests: iotests: fail hard if not run via "check"

Commit Message

Sascha Silbe April 14, 2016, 11:32 a.m. UTC

Running an iotests-based Python test directly might appear to work,
but may fail in subtle ways and is insecure:

- It creates files with predictable file names in a world-writable
  location (/var/tmp).

- Tests expect the environment to be set up by check. E.g. 041 and 055
  may take the wrong code paths if QEMU_DEFAULT_MACHINE is not
  set. This can lead to false negatives.

Instead fail hard and tell the user we want to be run via "check".

Signed-off-by: Sascha Silbe <[e-mail]>
Reviewed-by: Bo Tu <[e-mail]>
---
It's possible to fix iotests.py to work even outside of check, but
that requires reimplementing parts of what check currently does. I'd
prefer not to do that this late in the cycle.

It would be nice to have this in 2.6, just in case someone even tries
running a Python-based test directly instead of using ./check like for
the shell-based tests. But it's not crucial.
---
 tests/qemu-iotests/iotests.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Max Reitz April 14, 2016, 10:11 p.m. UTC | #1

On 14.04.2016 13:32, Sascha Silbe wrote:
> Running an iotests-based Python test directly might appear to work,
> but may fail in subtle ways and is insecure:
> 
> - It creates files with predictable file names in a world-writable
>   location (/var/tmp).
> 
> - Tests expect the environment to be set up by check. E.g. 041 and 055
>   may take the wrong code paths if QEMU_DEFAULT_MACHINE is not
>   set. This can lead to false negatives.
> 
> Instead fail hard and tell the user we want to be run via "check".
> 
> Signed-off-by: Sascha Silbe <[e-mail]>
> Reviewed-by: Bo Tu <[e-mail]>
> ---
> It's possible to fix iotests.py to work even outside of check, but
> that requires reimplementing parts of what check currently does. I'd
> prefer not to do that this late in the cycle.
> 
> It would be nice to have this in 2.6, just in case someone even tries
> running a Python-based test directly instead of using ./check like for
> the shell-based tests. But it's not crucial.
> ---
>  tests/qemu-iotests/iotests.py | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 0c0b533..8c7138f 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -46,7 +46,7 @@ if os.environ.get('QEMU_OPTIONS'):
>  
>  imgfmt = os.environ.get('IMGFMT', 'raw')
>  imgproto = os.environ.get('IMGPROTO', 'file')
> -test_dir = os.environ.get('TEST_DIR', '/var/tmp')
> +test_dir = os.environ.get('TEST_DIR')
>  output_dir = os.environ.get('OUTPUT_DIR', '.')
>  cachemode = os.environ.get('CACHEMODE')
>  qemu_default_machine = os.environ.get('QEMU_DEFAULT_MACHINE')
> @@ -441,6 +441,10 @@ def verify_quorum():
>  def main(supported_fmts=[], supported_oses=['linux']):
>      '''Run tests'''
>  
> +    if test_dir is None or qemu_default_machine is None:

I think checking test_dir would have been sufficient; this makes it look
like it might want to be a complete list of variables that have to be
set, but then "cachemode" is missing.

> +        sys.stderr.write('Please run this test via ./check\n')

Or not ./, for people who have separate build and source trees, you
don't want to invoke check in the source tree. ;-)

I'm not sure whether I'd want a v2 just because of this. It's
bike-shedding, but now that I think about it I guess I think I do.

So would you be so kind as to replace the "./check" by "the check
script"? :-)

(I don't particularly care about whether you change the condition used
in the if statement, though.)

Max

> +        sys.exit(os.EX_USAGE)
> +
>      debug = '-d' in sys.argv
>      verbosity = 1
>      verify_image_format(supported_fmts)
>

Markus Armbruster April 18, 2016, 7:19 a.m. UTC | #2

Sascha Silbe <[e-mail]> writes:

> Running an iotests-based Python test directly might appear to work,
> but may fail in subtle ways and is insecure:
>
> - It creates files with predictable file names in a world-writable
>   location (/var/tmp).
>
> - Tests expect the environment to be set up by check. E.g. 041 and 055
>   may take the wrong code paths if QEMU_DEFAULT_MACHINE is not
>   set. This can lead to false negatives.
>
> Instead fail hard and tell the user we want to be run via "check".

Are the problems serious enough to warrant rejecting the attempt to run
the tests directly outright?

Judging from your description, I'm inclined to "no".

> Signed-off-by: Sascha Silbe <[e-mail]>
> Reviewed-by: Bo Tu <[e-mail]>
> ---
> It's possible to fix iotests.py to work even outside of check, but
> that requires reimplementing parts of what check currently does. I'd
> prefer not to do that this late in the cycle.
>
> It would be nice to have this in 2.6, just in case someone even tries
> running a Python-based test directly instead of using ./check like for
> the shell-based tests. But it's not crucial.
> ---
>  tests/qemu-iotests/iotests.py | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 0c0b533..8c7138f 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -46,7 +46,7 @@ if os.environ.get('QEMU_OPTIONS'):
>  
>  imgfmt = os.environ.get('IMGFMT', 'raw')
>  imgproto = os.environ.get('IMGPROTO', 'file')
> -test_dir = os.environ.get('TEST_DIR', '/var/tmp')
> +test_dir = os.environ.get('TEST_DIR')
>  output_dir = os.environ.get('OUTPUT_DIR', '.')
>  cachemode = os.environ.get('CACHEMODE')
>  qemu_default_machine = os.environ.get('QEMU_DEFAULT_MACHINE')
> @@ -441,6 +441,10 @@ def verify_quorum():
>  def main(supported_fmts=[], supported_oses=['linux']):
>      '''Run tests'''
>  
> +    if test_dir is None or qemu_default_machine is None:
> +        sys.stderr.write('Please run this test via ./check\n')
> +        sys.exit(os.EX_USAGE)
> +
>      debug = '-d' in sys.argv
>      verbosity = 1
>      verify_image_format(supported_fmts)

Aha, you're not actually rejecting the attempt to run the tests
directly, you're merely requiring two environment variables.  That's
okay with me.  However, the commit message should explain it.  I'd also
rephrase the error message to something like

    <program_name>: TEST_DIR and QEMU_DEFAULT_MACHINE must be set in environment
    The easiest way to do that is running the test via the check script.

Sascha Silbe April 19, 2016, 11:59 a.m. UTC | #3

Dear Markus,

Markus Armbruster <[e-mail]> writes:

>> Running an iotests-based Python test directly might appear to work,
>> but may fail in subtle ways and is insecure:
>>
>> - It creates files with predictable file names in a world-writable
>>   location (/var/tmp).
>>
>> - Tests expect the environment to be set up by check. E.g. 041 and 055
>>   may take the wrong code paths if QEMU_DEFAULT_MACHINE is not
>>   set. This can lead to false negatives.
>>
>> Instead fail hard and tell the user we want to be run via "check".
>
> Are the problems serious enough to warrant rejecting the attempt to run
> the tests directly outright?
>
> Judging from your description, I'm inclined to "no".

Having tests do unexpected things and / or fail silently is "serious" in
my book. After all, what's the value of tests if they don't yell when
something is broken?


[tests/qemu-iotests/iotests.py]
[...]
>> +    if test_dir is None or qemu_default_machine is None:
>> +        sys.stderr.write('Please run this test via ./check\n')
>> +        sys.exit(os.EX_USAGE)
>> +
>
> Aha, you're not actually rejecting the attempt to run the tests
> directly, you're merely requiring two environment variables.  That's
> okay with me. [...]

I should probably add a comment that these two environment variables are
merely used as proxies that indicate we haven't been run via
"check". They are the ones where there isn't any useful default value we
could fall back, but without a full audit we can't tell what else some
of the tests may expect that would normally be set up by "check" (or at
least I can't).

Sascha

Sascha Silbe April 19, 2016, 12:22 p.m. UTC | #4

Dear Max,

Max Reitz <[e-mail]> writes:

> On 14.04.2016 13:32, Sascha Silbe wrote:
[tests/qemu-iotests/iotests.py]
[...]
>>  def main(supported_fmts=[], supported_oses=['linux']):
>>      '''Run tests'''
>>  
>> +    if test_dir is None or qemu_default_machine is None:
>
> I think checking test_dir would have been sufficient; this makes it look
> like it might want to be a complete list of variables that have to be
> set, but then "cachemode" is missing.

Markus Armbruster basically pointed out the same issue, so how about
this version:

    # We are using TEST_DIR and QEMU_DEFAULT_MACHINE as proxies to
    # indicate that we're not being run via "check". There may be
    # other things set up by "check" that individual test cases rely
    # on.
    if test_dir is None or qemu_default_machine is None:
        sys.stderr.write('Please run this test via the "check" script\n')
        sys.exit(os.EX_USAGE)


>> +        sys.stderr.write('Please run this test via ./check\n')
>
> Or not ./, for people who have separate build and source trees, you
> don't want to invoke check in the source tree. ;-)

Yeah, was thinking about that, but ultimately considered ./check to be
the best indication of a) "check" being the name of a script and b)
residing in the same directory as the test. But I don't care much about
this, so see the above version.

Sascha

Markus Armbruster April 19, 2016, 12:25 p.m. UTC | #5

Sascha Silbe <[e-mail]> writes:

> Dear Markus,
>
> Markus Armbruster <[e-mail]> writes:
>
>>> Running an iotests-based Python test directly might appear to work,
>>> but may fail in subtle ways and is insecure:
>>>
>>> - It creates files with predictable file names in a world-writable
>>>   location (/var/tmp).
>>>
>>> - Tests expect the environment to be set up by check. E.g. 041 and 055
>>>   may take the wrong code paths if QEMU_DEFAULT_MACHINE is not
>>>   set. This can lead to false negatives.
>>>
>>> Instead fail hard and tell the user we want to be run via "check".
>>
>> Are the problems serious enough to warrant rejecting the attempt to run
>> the tests directly outright?
>>
>> Judging from your description, I'm inclined to "no".
>
> Having tests do unexpected things and / or fail silently is "serious" in
> my book. After all, what's the value of tests if they don't yell when
> something is broken?
>
>
> [tests/qemu-iotests/iotests.py]
> [...]
>>> +    if test_dir is None or qemu_default_machine is None:
>>> +        sys.stderr.write('Please run this test via ./check\n')
>>> +        sys.exit(os.EX_USAGE)
>>> +
>>
>> Aha, you're not actually rejecting the attempt to run the tests
>> directly, you're merely requiring two environment variables.  That's
>> okay with me. [...]
>
> I should probably add a comment that these two environment variables are
> merely used as proxies that indicate we haven't been run via
> "check". They are the ones where there isn't any useful default value we
> could fall back, but without a full audit we can't tell what else some
> of the tests may expect that would normally be set up by "check" (or at
> least I can't).

Say you had an accurate way to find out whether we're running under
"check".  You could then reject any attempt to run the test directly.
I'd oppose that.

It's okay to have test wrapper scripts to configure the tests just so.
It's okay to tell people to use them.  But "you can't do that, Dave" is
not okay.

It's okay to require certain environment variables to be set.  That's
what your patch does (but your commit message doesn't quite say).

It would not be okay to interfere with my ability to run tests the way
*I* want.  If a test breaks when I run it my way, I get to keep the
pieces.

Sascha Silbe April 19, 2016, 4:49 p.m. UTC | #6

Dear Markus,

Markus Armbruster <[e-mail]> writes:

> Say you had an accurate way to find out whether we're running under
> "check".  You could then reject any attempt to run the test directly.
> I'd oppose that.
>
> It's okay to have test wrapper scripts to configure the tests just so.
> It's okay to tell people to use them.  But "you can't do that, Dave" is
> not okay. [...]

AFAICT the environment in which the individual test cases run isn't
well-defined. Currently it's indirectly defined by whatever "check"
does.

The goal of the patch is to catch unwary developers invoking the tests
directly from the command line, providing them with useful advice. If
somebody wants to write another test runner (in place of "check"), it's
their responsibility to set up the environment appropriately. (They
could even set an environment variable "I_AM_CHECK=yes" if that's part
of the environment the tests expect).

I'd be perfectly fine with defining the environment more clearly and
possibly extending the implementation to allow individual test cases to
be invoked directly (without a test runner like "check"). But that would
be 2.7 material.

Sascha

Max Reitz April 19, 2016, 7:06 p.m. UTC | #7

On 19.04.2016 14:22, Sascha Silbe wrote:
> Dear Max,
> 
> Max Reitz <[e-mail]> writes:
> 
>> On 14.04.2016 13:32, Sascha Silbe wrote:
> [tests/qemu-iotests/iotests.py]
> [...]
>>>  def main(supported_fmts=[], supported_oses=['linux']):
>>>      '''Run tests'''
>>>  
>>> +    if test_dir is None or qemu_default_machine is None:
>>
>> I think checking test_dir would have been sufficient; this makes it look
>> like it might want to be a complete list of variables that have to be
>> set, but then "cachemode" is missing.
> 
> Markus Armbruster basically pointed out the same issue, so how about
> this version:
> 
>     # We are using TEST_DIR and QEMU_DEFAULT_MACHINE as proxies to
>     # indicate that we're not being run via "check". There may be
>     # other things set up by "check" that individual test cases rely
>     # on.
>     if test_dir is None or qemu_default_machine is None:
>         sys.stderr.write('Please run this test via the "check" script\n')
>         sys.exit(os.EX_USAGE)

I'm OK with that. I can imagine Markus isn't, because it's implying that
you should only run this test through "check", whereas Markus says that
maybe you have your own script and that is fine, too.

I think two things:

1. I'm not sure why you would want your own script. If the check script
isn't good enough, extend it.

2. If you do want your own script, I guess setting up the necessary
environment for each test is complicated enough to require you to know
what you're doing. And if you know what you're doing, you won't really
care about the wording of this warning anyway.

I think this is just a warning for unwary users who just try to execute
a python test directly because they think that maybe they can. And then
this line is just telling them that no, that is not how the test is
supposed to be run; instead, it tells them the most convenient and
common way to run it.

I think it would be confusing if we printed the exact technical details
here which basically nobody cares about anyway.

So I'm completely fine with this version.

>>> +        sys.stderr.write('Please run this test via ./check\n')
>>
>> Or not ./, for people who have separate build and source trees, you
>> don't want to invoke check in the source tree. ;-)
> 
> Yeah, was thinking about that, but ultimately considered ./check to be
> the best indication of a) "check" being the name of a script and b)
> residing in the same directory as the test. But I don't care much about
> this, so see the above version.

Yes, that looks fine to me.

Regarding b): If you separate the build tree from the source tree, you
have to run the check script from the build tree. During the build
process, qemu will in fact automatically create a symlink in the build
tree. Therefore, in that case, you don't want to execute "./check" in
the same directory as the test is in.

Max

Sascha Silbe April 19, 2016, 7:32 p.m. UTC | #8

Dear Max,

Max Reitz <[e-mail]> writes:

> On 19.04.2016 14:22, Sascha Silbe wrote:
[...]
>>     # We are using TEST_DIR and QEMU_DEFAULT_MACHINE as proxies to
>>     # indicate that we're not being run via "check". There may be
>>     # other things set up by "check" that individual test cases rely
>>     # on.
>>     if test_dir is None or qemu_default_machine is None:
>>         sys.stderr.write('Please run this test via the "check" script\n')
>>         sys.exit(os.EX_USAGE)
>
> I'm OK with that. I can imagine Markus isn't, because it's implying that
> you should only run this test through "check", whereas Markus says that
> maybe you have your own script and that is fine, too.
[...]
> 2. If you do want your own script, I guess setting up the necessary
> environment for each test is complicated enough to require you to know
> what you're doing. And if you know what you're doing, you won't really
> care about the wording of this warning anyway.
>
> I think this is just a warning for unwary users who just try to execute
> a python test directly because they think that maybe they can. [...]

Seems we're on the same page. For those who want to write their own test
runners, the comment in the source indicates why we recommend running
via "check", implying that they'd need to set up the environment just
like "check" does. That's more or less the best we can do in the current
situation, so I'll spin up a v2 with the above code block. If Markus
would like it explicitly spelled out in the comment or the error
message, that'd be fine with me, too. Feel free to amend locally before
merging in that case.


[...]
> Regarding b): If you separate the build tree from the source tree, you
> have to run the check script from the build tree. During the build
> process, qemu will in fact automatically create a symlink in the build
> tree. Therefore, in that case, you don't want to execute "./check" in
> the same directory as the test is in.

Whereas there probably are no symlinks to the individual test
cases. Good point.

Sascha

Markus Armbruster April 20, 2016, 6:55 a.m. UTC | #9

Max Reitz <[e-mail]> writes:

> On 19.04.2016 14:22, Sascha Silbe wrote:
>> Dear Max,
>> 
>> Max Reitz <[e-mail]> writes:
>> 
>>> On 14.04.2016 13:32, Sascha Silbe wrote:
>> [tests/qemu-iotests/iotests.py]
>> [...]
>>>>  def main(supported_fmts=[], supported_oses=['linux']):
>>>>      '''Run tests'''
>>>>  
>>>> +    if test_dir is None or qemu_default_machine is None:
>>>
>>> I think checking test_dir would have been sufficient; this makes it look
>>> like it might want to be a complete list of variables that have to be
>>> set, but then "cachemode" is missing.
>> 
>> Markus Armbruster basically pointed out the same issue, so how about
>> this version:
>> 
>>     # We are using TEST_DIR and QEMU_DEFAULT_MACHINE as proxies to
>>     # indicate that we're not being run via "check". There may be
>>     # other things set up by "check" that individual test cases rely
>>     # on.
>>     if test_dir is None or qemu_default_machine is None:
>>         sys.stderr.write('Please run this test via the "check" script\n')
>>         sys.exit(os.EX_USAGE)
>
> I'm OK with that. I can imagine Markus isn't, because it's implying that
> you should only run this test through "check", whereas Markus says that
> maybe you have your own script and that is fine, too.
>
> I think two things:
>
> 1. I'm not sure why you would want your own script. If the check script
> isn't good enough, extend it.
>
> 2. If you do want your own script, I guess setting up the necessary
> environment for each test is complicated enough to require you to know
> what you're doing. And if you know what you're doing, you won't really
> care about the wording of this warning anyway.
>
> I think this is just a warning for unwary users who just try to execute
> a python test directly because they think that maybe they can. And then
> this line is just telling them that no, that is not how the test is
> supposed to be run; instead, it tells them the most convenient and
> common way to run it.
>
> I think it would be confusing if we printed the exact technical details
> here which basically nobody cares about anyway.
>
> So I'm completely fine with this version.

As Max predicted, I'm not :)

'Please run this test via the "check" script' isn't an error message,
it's advice.  Advice is appreciated, but an error message must come
first.  I reiterate my proposal to use something like

    <program_name>: TEST_DIR and QEMU_DEFAULT_MACHINE must be set in environment
    The easiest way to do that is running the test via the check script.

or

    <program_name>: TEST_DIR and QEMU_DEFAULT_MACHINE must be set in environment
    Please run this test via the "check" script

[...]

Sascha Silbe April 20, 2016, 8:37 a.m. UTC | #10

Dear Markus,

Markus Armbruster <[e-mail]> writes:

> 'Please run this test via the "check" script' isn't an error message,
> it's advice.  Advice is appreciated, but an error message must come
> first.  I reiterate my proposal to use something like
>
>     <program_name>: TEST_DIR and QEMU_DEFAULT_MACHINE must be set in environment
>     The easiest way to do that is running the test via the check script.

But I don't know how the environment (again, broader sense than just
environment variables) must be set up; that's part of the problem. I
know that at least the TEST_DIR and QEMU_DEFAULT_MACHINE environment
variables must be set, but listing those in the error message gives an
impression that it's all the user needs to do. Bad advice is worse than
no advice.

We could write something like "The TEST_DIR and QEMU_DEFAULT_MACHINE
environment variables must be set and there may be other things that the
tests may expect to have been set up. See what the 'check' script does
if you want to invoke the tests some other way than running 'check'.",
but I don't really see the additional value to the user.

Sascha

Kevin Wolf April 20, 2016, 8:38 a.m. UTC | #11

Am 19.04.2016 um 18:49 hat Sascha Silbe geschrieben:
> Dear Markus,
> 
> Markus Armbruster <[e-mail]> writes:
> 
> > Say you had an accurate way to find out whether we're running under
> > "check".  You could then reject any attempt to run the test directly.
> > I'd oppose that.
> >
> > It's okay to have test wrapper scripts to configure the tests just so.
> > It's okay to tell people to use them.  But "you can't do that, Dave" is
> > not okay. [...]
> 
> AFAICT the environment in which the individual test cases run isn't
> well-defined. Currently it's indirectly defined by whatever "check"
> does.
> 
> The goal of the patch is to catch unwary developers invoking the tests
> directly from the command line, providing them with useful advice. If
> somebody wants to write another test runner (in place of "check"), it's
> their responsibility to set up the environment appropriately. (They
> could even set an environment variable "I_AM_CHECK=yes" if that's part
> of the environment the tests expect).
> 
> I'd be perfectly fine with defining the environment more clearly and
> possibly extending the implementation to allow individual test cases to
> be invoked directly (without a test runner like "check"). But that would
> be 2.7 material.

At this point in the 2.6 release cycle, this series is 2.7 material
anyway. It's critical fixes only now.

Kevin

Sascha Silbe April 20, 2016, 8:51 a.m. UTC | #12

Dear Kevin,

Kevin Wolf <[e-mail]> writes:

> At this point in the 2.6 release cycle, this series is 2.7 material
> anyway. It's critical fixes only now.

Fair enough.

Sascha

Patch

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 0c0b533..8c7138f 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -46,7 +46,7 @@  if os.environ.get('QEMU_OPTIONS'):
 
 imgfmt = os.environ.get('IMGFMT', 'raw')
 imgproto = os.environ.get('IMGPROTO', 'file')
-test_dir = os.environ.get('TEST_DIR', '/var/tmp')
+test_dir = os.environ.get('TEST_DIR')
 output_dir = os.environ.get('OUTPUT_DIR', '.')
 cachemode = os.environ.get('CACHEMODE')
 qemu_default_machine = os.environ.get('QEMU_DEFAULT_MACHINE')
@@ -441,6 +441,10 @@  def verify_quorum():
 def main(supported_fmts=[], supported_oses=['linux']):
     '''Run tests'''
 
+    if test_dir is None or qemu_default_machine is None:
+        sys.stderr.write('Please run this test via ./check\n')
+        sys.exit(os.EX_USAGE)
+
     debug = '-d' in sys.argv
     verbosity = 1
     verify_image_format(supported_fmts)