Opened 5 years ago

Closed 5 years ago

#14478 closed enhancement (fixed)

Zero length keys test improvements

Reported by: cypherpunks Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The zero length keys could use some small improvements, such as;

  1. Remove the temporary directories that are generated. It prevents the main temporary directory from growing over time after each script execution. The solution would be a trap that removes them when the script exits.
    diff --git a/src/test/zero_length_keys.sh b/src/test/zero_length_keys.sh
    index 3a99ca1..2545734 100755
    --- a/src/test/zero_length_keys.sh
    +++ b/src/test/zero_length_keys.sh
    @@ -26,6 +26,8 @@ if [ $# -lt 1 ]; then
     fi
     
     export DATA_DIR=`mktemp -d -t tor_zero_length_keys.XXXXXX`
    +trap "rm -rf "$DATA_DIR"" 0
    +
     # DisableNetwork means that the ORPort won't actually be opened.
     # 'ExitRelay 0' suppresses a warning.
     TOR="./src/or/tor --hush --DisableNetwork 1 --ShutdownWaitLength 0 --ORPort 12345 --ExitRelay 0"
    
  2. Do not export the variable DATA_DIR because it is useless and stop using the old style command substitution because it has cases of undefined results [0]. Also Bash mentions a preference towards the newer $() style [1].
    diff --git a/src/test/zero_length_keys.sh b/src/test/zero_length_keys.sh
    index 3a99ca1..ede7df5 100755
    --- a/src/test/zero_length_keys.sh
    +++ b/src/test/zero_length_keys.sh
    @@ -25,7 +25,8 @@ if [ $# -lt 1 ]; then
       exit $?
     fi
     
    -export DATA_DIR=`mktemp -d -t tor_zero_length_keys.XXXXXX`
    +DATA_DIR="$(mktemp -d -t tor_zero_length_keys.XXXXXX)"
    +
     # DisableNetwork means that the ORPort won't actually be opened.
     # 'ExitRelay 0' suppresses a warning.
     TOR="./src/or/tor --hush --DisableNetwork 1 --ShutdownWaitLength 0 --ORPort 12345 --ExitRelay 0"
    
  3. Because i am unsure which platforms/shells need to be supported, this improvement may be more work than the satisfaction of correctness it gives. In GNU Coreutils the mktemp program marks the -t option as deprecated [2]. On other platforms the option is not deprecated. A list of options of the several mktemp implementations is available on StackOverflow. The question is whether the script needs to be platform-aware so it can avoid deprecated options or not.

[0] http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_06_03
[1] https://www.gnu.org/software/bash/manual/bashref.html#Major-Differences-From-The-Bourne-Shell (search for $())
[2] https://www.gnu.org/software/coreutils/manual/html_node/mktemp-invocation.html#mktemp-invocation

PS: The patches are just suggestions, if others agree i can merge them together into one patch and upload a patch file for easier merging.

Child Tickets

Change History (12)

comment:1 Changed 5 years ago by nickm

Milestone: Tor: unspecifiedTor: 0.2.6.x-final

2 and 3 seem like a good idea.

I like the idea behind 1, but I'm worried about the "rm -rf". Call me paranoid, but "rm -rf" is just *asking* for trouble. Can we define a function to get called when the script exits, and rm the specific files instead? If not we should at least make sure that the filename returned by mktemp is nonempty and more or less as expected, I think.

comment:2 Changed 5 years ago by nickm

Status: newneeds_review

comment:3 in reply to:  1 ; Changed 5 years ago by cypherpunks

Replying to nickm:

2 and 3 seem like a good idea.

I like the idea behind 1, but I'm worried about the "rm -rf". Call me paranoid, but "rm -rf" is just *asking* for trouble. Can we define a function to get called when the script exits, and rm the specific files instead? If not we should at least make sure that the filename returned by mktemp is nonempty and more or less as expected, I think.

While writing the function i ran into more possible improvements. First, the negative exit values seem to produce errors because negative values are an ''illegal number''. Can someone verify this?

some@other:shell$ sh
$ exit -1
sh: 1: exit: Illegal number: -1
$

Note that after the error, the shell (sh) does not actually exit.

Second, parameter expansion uses quotes to ensure it is properly expanded. By using ${} the quotes become unnecessary and can be ''saved'' for quoting the entire string (which should be always be done, not just in cases with spaces, for consistency) without the need for two double quotes.

comment:4 Changed 5 years ago by cypherpunks

The third additional improvement is splitting up multiple test arguments into single arguments. This is to avoid undefined behavior with >4 arguments as stated in the POSIX documentation on test [0].

[0] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html

comment:5 in reply to:  3 Changed 5 years ago by nickm

Status: needs_reviewneeds_revision

Replying to cypherpunks:

Replying to nickm:

2 and 3 seem like a good idea.

I like the idea behind 1, but I'm worried about the "rm -rf". Call me paranoid, but "rm -rf" is just *asking* for trouble. Can we define a function to get called when the script exits, and rm the specific files instead? If not we should at least make sure that the filename returned by mktemp is nonempty and more or less as expected, I think.

While writing the function i ran into more possible improvements. First, the negative exit values seem to produce errors because negative values are an ''illegal number''. Can someone verify this?

some@other:shell$ sh
$ exit -1
sh: 1: exit: Illegal number: -1
$

Note that after the error, the shell (sh) does not actually exit.

This behavior seems shell-dependent. Better not use -1 here.

Second, parameter expansion uses quotes to ensure it is properly expanded. By using ${} the quotes become unnecessary and can be ''saved'' for quoting the entire string (which should be always be done, not just in cases with spaces, for consistency) without the need for two double quotes.

Seems fine to me.

comment:6 Changed 5 years ago by Sebastian

I added a branch with some your patches, bug14478 in my repo.

I haven't added the first idea yet, because of the reservations against mktemp followed by rm -rf. Adding a function like Nick suggested would be pretty hard, because who knows what kind of files tor will create during its lifetime (and some files it even creates and cleans up itself, unless it crashes). Nick, would it be OK to do a check "$DATADIR is not empty, the directory exists and it is empty as we start the test"?

I didn't include patch number three, because that seems a little unnecessary to me - the switch is deprecated, not removed after all. Detecting whether the switch is deprecated or required is difficult, just testing the platform isn't enough because one might have a different implementation in their path.

The issue about undefined behaviour I don't quite get. Is one really supposed to use -a only with variables? is [ -s $a -a b ] really unspecified behaviour?

comment:7 Changed 5 years ago by Sebastian

Status: needs_revisionneeds_review

Hrm, I'm not seeing how we can do any kind of sane checking of the string returned by mktemp. If the command invocation fails and it is empty, I am returning an error and a log message. I don't see how this could go wrong except by mktemp having a bug and returning a random directory, which we can't defend against afaict.

I added another patch on top.

comment:8 in reply to:  6 Changed 5 years ago by nickm

Replying to Sebastian:

The issue about undefined behaviour I don't quite get. Is one really supposed to use -a only with variables? is [ -s $a -a b ] really unspecified behaviour?

It doesn't work with old or weird shells, test -s "$a" && test -s "$b" is a little more portable.

Replying to Sebastian:
Hrm, I'm not seeing how we can do any kind of sane checking of the string returned by mktemp. If the command invocation fails and it is empty, I am returning an error and a log message. I don't see how this could go wrong except by mktemp having a bug and returning a random directory, which we can't defend against afaict.

Okay, I guess that's probably fine. Better make sure it's a directory too, and we should be okay.

I think this new thing won't work though: trap "rm -rf "$DATA_DIR"" 0 . Double-quotes don't really nest that way. Try running this with TMPDIR set to something with a space in it to test?

comment:9 Changed 5 years ago by Sebastian

you're right, thanks! I pushed a fixup commit, care to take a look now?

comment:10 Changed 5 years ago by nickm

Did you test +trap "rm -rf '$DATA_DIR'" 0 ? I think that single-quotes never interpolate.

comment:11 Changed 5 years ago by Sebastian

Yes, I did test it and it works for me in bash, zsh, and dash.

comment:12 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged; thanks!

Note: See TracTickets for help on using tickets.