xerial/snappy-java

Library files leftover when process exits abruptly

ivanyu opened this issue · 5 comments

ivanyu commented

Hello!

I faced the following issue on Linux (assume it will be the same on other OSes).

If a process that uses snappy-java (I tried on the latest version 1.1.8.4) exists abruptly (for example, being killed, dies due to OOM), the .so file remains in /tmp as deleteOnExit doesn't have chance to run.
This is a problem if you have a process failing in a loop.

The solution seems rather simple to me:

  1. Replace UUID with the SHA-256 hash of the .so file.
  2. If the file in /tmp exists, check the hash and overwrite if the hash doesn't match.

If you will accept this solution, I will make a PR.

bokken commented
ivanyu commented

Thanks for your reply @bokken

I don't think deleting a dynamic library file will affect any process that has already loaded it. This is because deletion (as in rm or Java's File.delete) is not a real deletion, but just unlinking the directory entry, while the open file descriptors remain valid. The same applies to overwriting with mv.

This holds true on Linux and *nix, including MacOS. I checked how it's currently implemented on Windows and (without much details) it's not possible to delete a dynamic library file that is currently loaded by a process. (Sorry for not checking this in the first place.)

However, I didn't plan to delete the .so/.dll files on exit. As they should be the same every run, we can let them stay in the temp directory and just not unpack next time.

Overwriting is a different story. Originally I proposed to overwrite if the content doesn't match the hash, but this may actually be not a good idea by itself. This means something is fishy and someone has probably tampered with the file in /tmp or some unexpected error happened before, so it's better to fail and propose the user to deal with it in their way (e.g. by deleting the file).

A bit more refined plan would be (pseudo-code):

if (libraryFileExists) {
  if (!validateHash()) {
    logFatal("Library file content doesn't match its hash")
    exit(1)
  }
} else {
  writeFileAtomically()
}

void writeFileAtomically() {
  writeTempFile();
  rename();
}

How does it sound? I understand it's a bit more complex than the current approach, but not much.

xerial commented

@ivanyu Thanks for the proposal.

I basically agree that it's good if we can reuse the previously extracted files. In reality, however, using a predictable file name is vulnerable from a security perspective as an attacker can replace .so file with one that can run arbitrary code. If a machine is shared by multiple users, the risk is more serious.

As snappy-java is a user-side library, it should not have a regular installation process to put .so files into a controlled folder (e.g., /usr/lib) that cannot be modified by untrusted users. Validating hash values will reduce the risk, but as there is no reliable way to take a file lock right after the hash validation, it still isn't safe.

Another approach is adding some timestamp prefix to the .so file name so that we can clean up old .so files periodically based on the file name patterns.

bokken commented

#309 Describes a crash we observed some time after the snappy native library was deleted from under a running jvm.

Cleaning old files is good, but should only be done if you can determine no process has them open (i.e. lsof). Multiple different processes could be sharing the same temp location, so just because a file is present which the current classloader is not using does not mean it can/should be deleted.

ivanyu commented

Thanks for your reply @xerial. Your argument is valid, let's close this issue.