Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix race condition scheduling $tempdir deletion #216

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

EliahKagan
Copy link
Contributor

The cargo-zng script contained:

trap 'rm -rf "$tempdir"' 0 INT
tempdir="$(mktemp -d)"

That releases a resource that may occasionally not have been acquired.

In the rare case that the script would be interrupted before $tempdir was set, this would attempt to read the previous value of $tempdir. But if the calling environment had an environment variable of this name, then its value would have been inherited as the initial $tempdir in the script, and wrongly deleted.

The change made here puts the commands in the other order, so we only arrange for the deletion of $tempdir once the directory we want to delete exists and $tempdir will definitely expand to its name and not that of another directory.

In a sense, this trades one race condition for another, in that now the directory will not be deleted if the script is interrupted immediately after it is created. But this is not a problem, because it does not cause data loss, and the script already does not try to cover all ways the script can be terminated in the trap command.

The `cargo-zng` script contained:

    trap 'rm -rf "$tempdir"' 0 INT
    tempdir="$(mktemp -d)"

That releases a resource that may occasionally not have been
acquired.

In the rare case that the script would be interrupted before
`$tempdir` was set, this would attempt to read the previous value
of `$tempdir`. But if the calling environment had an environment
variable of this name, then its value would have been inherited as
the initial `$tempdir` in the script, and wrongly deleted.

The change made here puts the commands in the other order, so we
only arrange for the deletion of `$tempdir` once the directory we
want to delete exists and `$tempdir` will definitely expand to its
name and not that of another directory.

In a sense, this trades one race condition for another, in that
now the directory will not be deleted if the script is interrupted
immediately after it is created. But this is not a problem, because
it does not cause data loss, and the script already does not try to
cover all ways the script can be terminated in the `trap` command.
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for the fix, this is a critical one!

I never thought about this, but now once again realize that globals really are a problem.
This wouldn't have been possible if there was a function () {} that declares local tempdir, but it isn't very intuitive just for the 'local variable' feature.

@Byron Byron merged commit d39c1c5 into rust-lang:main Sep 6, 2024
48 checks passed
@EliahKagan EliahKagan deleted the protect-outer-tempdir branch September 6, 2024 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants