[v2,01/23] iotests: Introduce $SOCK_DIR
diff mbox series

Message ID 20191017133155.5327-2-mreitz@redhat.com
State New
Headers show
Series
  • iotests: Add and use $SOCK_DIR
Related show

Commit Message

Max Reitz Oct. 17, 2019, 1:31 p.m. UTC
Unix sockets generally have a maximum path length.  Depending on your
$TEST_DIR, it may be exceeded and then all tests that create and use
Unix sockets there may fail.

Circumvent this by adding a new scratch directory specifically for
Unix socket files.  It defaults to a temporary directory (mktemp -d)
that is completely removed after the iotests are done.

(By default, mktemp -d creates a /tmp/tmp.XXXXXXXXXX directory, which
should be short enough for our use cases.)

Use mkdir -p to create the directory (because it seems right), and do
the same for $TEST_DIR (because there is no reason for that to be
created in any different way).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/check | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Comments

Eric Blake Oct. 17, 2019, 2:52 p.m. UTC | #1
On 10/17/19 8:31 AM, Max Reitz wrote:
> Unix sockets generally have a maximum path length.  Depending on your
> $TEST_DIR, it may be exceeded and then all tests that create and use
> Unix sockets there may fail.
> 
> Circumvent this by adding a new scratch directory specifically for
> Unix socket files.  It defaults to a temporary directory (mktemp -d)
> that is completely removed after the iotests are done.
> 
> (By default, mktemp -d creates a /tmp/tmp.XXXXXXXXXX directory, which
> should be short enough for our use cases.)
> 
> Use mkdir -p to create the directory (because it seems right), and do
> the same for $TEST_DIR (because there is no reason for that to be
> created in any different way).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   tests/qemu-iotests/check | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)

> @@ -116,10 +117,14 @@ set_prog_path()
>   if [ -z "$TEST_DIR" ]; then
>           TEST_DIR=$PWD/scratch
>   fi
> +mkdir -p "$TEST_DIR" || _init_error 'Failed to create TEST_DIR'

This one seems fine. We are either using the user's name (and if it is 
pre-existing, not fail) or using a well-known name (if someone else 
slams in files into that directory in parallel with our test run, oh 
well).  But at least the well-known name is a directory that is probably 
already accessible only to the current user, not world-writable.

>   
> -if [ ! -e "$TEST_DIR" ]; then
> -        mkdir "$TEST_DIR"
> +tmp_sock_dir=false
> +if [ -z "$SOCK_DIR" ]; then
> +    SOCK_DIR=$(mktemp -d)
> +    tmp_sock_dir=true
>   fi
> +mkdir -p "$SOCK_DIR" || _init_error 'Failed to create SOCK_DIR'

Thinking about this again: if the user passed in a name, we probably 
want to use it no matter whether the directory already exists (mkdir -p 
makes sense: either the directory did not exist, or the user is in 
charge of passing us a directory that they already secured).  But if we 
generate our own name in a world-writable location in /tmp, using mkdir 
-p means someone else can race us to the creation of the directory, and 
potentially populate it in a way to cause us a security hole while we 
execute our tests.

I would be a bit more comfortable with:

tmp_sock_dir=false
tmp_sock_opt=-p
if [ -z "$SOCK_DIR" ]; then
     SOCK_DIR=$(mktemp -d)
     tmp_sock_dir=true
     tmp_sock_opt=      # disable -p for our generated name
fi
mkdir $tmp_sock_opt "$SOCK_DIR" || _init_error 'Failed to create SOCK_DIR'
Max Reitz Oct. 18, 2019, 9:03 a.m. UTC | #2
On 17.10.19 16:52, Eric Blake wrote:
> On 10/17/19 8:31 AM, Max Reitz wrote:
>> Unix sockets generally have a maximum path length.  Depending on your
>> $TEST_DIR, it may be exceeded and then all tests that create and use
>> Unix sockets there may fail.
>>
>> Circumvent this by adding a new scratch directory specifically for
>> Unix socket files.  It defaults to a temporary directory (mktemp -d)
>> that is completely removed after the iotests are done.
>>
>> (By default, mktemp -d creates a /tmp/tmp.XXXXXXXXXX directory, which
>> should be short enough for our use cases.)
>>
>> Use mkdir -p to create the directory (because it seems right), and do
>> the same for $TEST_DIR (because there is no reason for that to be
>> created in any different way).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/check | 15 +++++++++++++--
>>   1 file changed, 13 insertions(+), 2 deletions(-)
> 
>> @@ -116,10 +117,14 @@ set_prog_path()
>>   if [ -z "$TEST_DIR" ]; then
>>           TEST_DIR=$PWD/scratch
>>   fi
>> +mkdir -p "$TEST_DIR" || _init_error 'Failed to create TEST_DIR'
> 
> This one seems fine. We are either using the user's name (and if it is
> pre-existing, not fail) or using a well-known name (if someone else
> slams in files into that directory in parallel with our test run, oh
> well).  But at least the well-known name is a directory that is probably
> already accessible only to the current user, not world-writable.
> 
>>   -if [ ! -e "$TEST_DIR" ]; then
>> -        mkdir "$TEST_DIR"
>> +tmp_sock_dir=false
>> +if [ -z "$SOCK_DIR" ]; then
>> +    SOCK_DIR=$(mktemp -d)
>> +    tmp_sock_dir=true
>>   fi
>> +mkdir -p "$SOCK_DIR" || _init_error 'Failed to create SOCK_DIR'
> 
> Thinking about this again: if the user passed in a name, we probably
> want to use it no matter whether the directory already exists (mkdir -p
> makes sense: either the directory did not exist, or the user is in
> charge of passing us a directory that they already secured).  But if we
> generate our own name in a world-writable location in /tmp, using mkdir
> -p means someone else can race us to the creation of the directory, and
> potentially populate it in a way to cause us a security hole while we
> execute our tests.

I don’t quite see how this is a security hole.  mktemp -d creates the
directory, so noone can race us.

Max

> I would be a bit more comfortable with:
> 
> tmp_sock_dir=false
> tmp_sock_opt=-p
> if [ -z "$SOCK_DIR" ]; then
>     SOCK_DIR=$(mktemp -d)
>     tmp_sock_dir=true
>     tmp_sock_opt=      # disable -p for our generated name
> fi
> mkdir $tmp_sock_opt "$SOCK_DIR" || _init_error 'Failed to create SOCK_DIR'
>
Eric Blake Oct. 18, 2019, 1:30 p.m. UTC | #3
On 10/18/19 4:03 AM, Max Reitz wrote:

>>>    -if [ ! -e "$TEST_DIR" ]; then
>>> -        mkdir "$TEST_DIR"
>>> +tmp_sock_dir=false
>>> +if [ -z "$SOCK_DIR" ]; then
>>> +    SOCK_DIR=$(mktemp -d)
>>> +    tmp_sock_dir=true
>>>    fi
>>> +mkdir -p "$SOCK_DIR" || _init_error 'Failed to create SOCK_DIR'
>>
>> Thinking about this again: if the user passed in a name, we probably
>> want to use it no matter whether the directory already exists (mkdir -p
>> makes sense: either the directory did not exist, or the user is in
>> charge of passing us a directory that they already secured).  But if we
>> generate our own name in a world-writable location in /tmp, using mkdir
>> -p means someone else can race us to the creation of the directory, and
>> potentially populate it in a way to cause us a security hole while we
>> execute our tests.
> 
> I don’t quite see how this is a security hole.  mktemp -d creates the
> directory, so noone can race us.

Aha - I confused 'mktemp -u' (necessary for creating a socket name) and 
'mktemp -d' (for directories).  With that confusion cleared up, yes, the 
directory is safely created (or else the burden is on the caller), so:

Reviewed-by: Eric Blake <eblake@redhat.com>

Patch
diff mbox series

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 588c453a94..71fe38834e 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -97,6 +97,7 @@  IMGFMT        -- $FULL_IMGFMT_DETAILS
 IMGPROTO      -- $IMGPROTO
 PLATFORM      -- $FULL_HOST_DETAILS
 TEST_DIR      -- $TEST_DIR
+SOCK_DIR      -- $SOCK_DIR
 SOCKET_SCM_HELPER -- $SOCKET_SCM_HELPER
 
 EOF
@@ -116,10 +117,14 @@  set_prog_path()
 if [ -z "$TEST_DIR" ]; then
         TEST_DIR=$PWD/scratch
 fi
+mkdir -p "$TEST_DIR" || _init_error 'Failed to create TEST_DIR'
 
-if [ ! -e "$TEST_DIR" ]; then
-        mkdir "$TEST_DIR"
+tmp_sock_dir=false
+if [ -z "$SOCK_DIR" ]; then
+    SOCK_DIR=$(mktemp -d)
+    tmp_sock_dir=true
 fi
+mkdir -p "$SOCK_DIR" || _init_error 'Failed to create SOCK_DIR'
 
 diff="diff -u"
 verbose=false
@@ -534,6 +539,7 @@  if [ -z "$SAMPLE_IMG_DIR" ]; then
 fi
 
 export TEST_DIR
+export SOCK_DIR
 export SAMPLE_IMG_DIR
 
 if [ -s $tmp.list ]
@@ -716,6 +722,11 @@  END        { if (NR > 0) {
     rm -f "${TEST_DIR}"/*.out "${TEST_DIR}"/*.err "${TEST_DIR}"/*.time
     rm -f "${TEST_DIR}"/check.pid "${TEST_DIR}"/check.sts
     rm -f $tmp.*
+
+    if $tmp_sock_dir
+    then
+        rm -rf "$SOCK_DIR"
+    fi
 }
 
 trap "_wrapup; exit \$status" 0 1 2 3 15