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.

Sign up for GitHub

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

Added a check for whether files can be created and executed within the configured tmp-dir #8951

Merged
merged 4 commits into from
Oct 18, 2024
Merged

Added a check for whether files can be created and executed within the configured tmp-dir #8951

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e608b05
Added a couple warnings for situations that could cause the tmp-dir t…
KevinCLydon Aug 1, 2024
6ba4be7
Minor formatting change
KevinCLydon Aug 13, 2024
29100fd
Modified the temp dir execution checking to only use nio
KevinCLydon Aug 13, 2024
4c123d0
Moved the tempdir exec permission checking into its own method
KevinCLydon Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions src/main/java/org/broadinstitute/hellbender/cmdline/CommandLineProgram.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import java.io.IOException;
import java.net.InetAddress;
import java.nio.file.*;
import java.nio.file.attribute.PosixFilePermission;
import java.text.DecimalFormat;
import java.time.Duration;
import java.time.ZonedDateTime;
Expand Down Expand Up @@ -165,8 +166,44 @@ public Object instanceMainPostParseArgs() {

// set the temp directory as a java property, checking for existence and read/write access
final Path p = tmpDir.toPath();
Path tempFilePath = null;
try {
p.getFileSystem().provider().checkAccess(p, AccessMode.READ, AccessMode.WRITE);

// Warn if there's anything that prevents execution in the tmp dir because some tools need that
// This test relies on the file system supporting posix file permissions
if(p.getFileSystem().supportedFileAttributeViews().contains("posix")) {
Copy link
Member

Choose a reason for hiding this comment

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

So I would pull this whole bit out into a new method called something stupid like "tryToWriteAnExecutableFileAndWarnOnFailure". There are multiple levels of try / catch here and it's kind of confusing at the moment.

Copy link
Member Author

@KevinCLydon KevinCLydon Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'll move this into a separate function but I'm not sure there's a great way to fix the nested try / catch issue. I want the inner one because it specifically catches issues setting exec permissions on the temp file and I don't think that function throws granular-enough exceptions for me to easily be able to distinguish between an exception from that and an exception from like creating the file or getting the posix permissions. And I need the outer try / catch even in a new function so I can make sure the temp file gets deleted.

Copy link
Member

Choose a reason for hiding this comment

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

I see your point and it makes sense. I'm not sure we care that much about treating those cases differently though? Can we give meaningfully different advice if getting permissions failed vs if setting exec failed? Maybe can. If you think that's a useful distinction I'm not against having it split out. I do like it all encapsulated in a function that won't explode in any case though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think maybe it's not a worthwhile distinction

Copy link
Member

Choose a reason for hiding this comment

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

A small and annoying caveat here, the HttpFileSystem throw UnsupportedOperation if you ask it this. (Which it probably should and we should fix).

Copy link
Member Author

Choose a reason for hiding this comment

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

What should be the course of action if that happens? Catch it and move on?

Copy link
Member

Choose a reason for hiding this comment

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

Well, we won't even get to here in that case, since we've already exploded on the check above due to not supporting write access. Just a weird edge case.

Copy link
Member

Choose a reason for hiding this comment

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

I think catch and continue was the right choice.

// Write an empty file to the tempdir
tempFilePath = Files.createTempFile(p, "gatk_exec_test", null);
Copy link
Member

Choose a reason for hiding this comment

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

Lets move this and the permissions getting into the try block. I think we don't want additional potential exceptions propagating up here. This is a warning only situation.

// Add execute permissions
final Set<PosixFilePermission> executePermissions = EnumSet.of(
PosixFilePermission.OWNER_EXECUTE,
PosixFilePermission.GROUP_EXECUTE,
PosixFilePermission.OTHERS_EXECUTE
);
final Set<PosixFilePermission> newPermissions = Files.getPosixFilePermissions(tempFilePath);
newPermissions.addAll(executePermissions);
try{
Files.setPosixFilePermissions(tempFilePath, newPermissions);
if(!Files.isExecutable(tempFilePath)) {
logger.warn(
"User has permissions to create executable files within the configured temporary directory, " +
"but cannot execute those files. It is possible the directory has been mounted using the " +
"'noexec' flag. This can cause issues for some GATK tools. You can specify a different " +
"directory using --tmp-dir"
);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I always prefer braces styled like

} catch () {
   ...
} finally {

instead of

} 
catch () {
 ...
}
finally {

It's not big deal either way and we definitely have examples of both, but if you're not ideologically I'd say use braces on the same line.

catch(IOException e) {
Copy link
Member

Choose a reason for hiding this comment

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

We might want to catch general "Exception" here. That's generally bad practice, but in this case we really want this to be only a warning I think and not crash GATK if something weird gets thrown.

logger.warn(
Copy link
Member

Choose a reason for hiding this comment

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

We should probably attach the exception here. Since we want a sane human readable warning we might want to put an additional logger.debug(e) beneath this instead of logging it with the user warning.

"Cannot create executable files within the configured temporary directory. It is possible " +
"this user does not have the proper permissions to execute files within this directory. " +
"This can cause issues for some GATK tools. You can specify a different directory using " +
"--tmp-dir"
);
}
Copy link
Member

Choose a reason for hiding this comment

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

Lets add a logger.debug() line in the case where posix permissions aren't available.

}

System.setProperty("java.io.tmpdir", IOUtils.getAbsolutePathWithoutFileProtocol(p));
} catch (final AccessDeniedException | NoSuchFileException e) {
// TODO: it may be that the program does not need a tmp dir
Expand All @@ -177,6 +214,16 @@ public Object instanceMainPostParseArgs() {
// other exceptions with the tmp directory
throw new UserException.BadTempDir(p, e.getMessage(), e);
}
finally {
Copy link
Member

Choose a reason for hiding this comment

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

I think we can move this finally block and attack it to the try where we are writing the file.

// Make sure we clean up the test file
try {
if (tempFilePath != null)
Copy link
Member

Choose a reason for hiding this comment

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

always {}

Files.deleteIfExists(tempFilePath);
}
catch(Exception e) {
logger.warn("Failed to delete temp file for testing temp dir", e);
}
}

//Set defaults (note: setting them here means they are not controllable by the user)
if (! useJdkDeflater) {
Expand Down
Loading