Stebalien / tempfile

Temporary file library for rust
http://stebalien.com/projects/tempfile-rs
Apache License 2.0
1.18k stars 117 forks source link

Permission issue on Android #235

Closed GregoryConrad closed 1 year ago

GregoryConrad commented 1 year ago

Hi! tempfile is a transitive dependency of another crate I've been using and I noticed Android (API level <33 from what it seems) has permission issues on many different devices/emulators when using the directory returned by std::env::tempdir, which, on Android, always returns /data/local/tmp.

Very unfortunately, there is no clear fix because the only way to access an application's specific temporary folder in Android is via context.getCacheDir(), which requires a context object from the application itself.

Instead of attempting a hacky fix for Android specifically, I think it'd be a good idea to introduce a new global in tempfile (which can be used by all platforms) that accepts an "override" temporary directory. If set, the global overrides the directory returned by std::env::tempdir for all future invocations of tempfile. This maintains 100% backwards compatibility on all platforms and also allows anyone to override the default directory, which is necessary on platforms like Android. (To be completely clear, I really am not a fan that such a change is required, but Android doesn't give you any other option; they really didn't consider native code when they designed that API.)

Was thinking something along the lines of:

static OVERRIDE_TMP_DIR: RwLock<Option<PathBuf>> = RwLock::new(None);

Was planning on std's RwLock since this crate already relies upon std. parking_lot would also be good, but it is another dependency. I'd imagine RwLock does have some slight overhead due to calling read() every time we currently call std::env::tempdir, but I would also imagine that overhead is not nearly as much as actually creating a temporary file, which is exactly what tempfile() does. Thus, I think the overhead introduced should be negligible (thread synchronization, especially with almost exclusively read()s, is not as expensive as filesystem access).

Would a PR for such a feature be accepted @Stebalien? Thanks.

Stebalien commented 1 year ago

Hm. That's annoying.

It looks like the standard library looks at the TMPDIR env variable on all platforms. Have you considered setting that? I.e., std::env::set_var("TMPDIR", ...).

Stebalien commented 1 year ago

That will have the benefit of applying to any library looking for the temporary directory.

GregoryConrad commented 1 year ago

@Stebalien Sure, I can see if that does the trick! That would be a really nice solution, assuming it works. However, std::env::tempdir is documented as only returning /data/local/tmp on Android, so I would still be a bit hesitant to relying upon an un-documented implementation detail like that (I wouldn't want my builds to start mysteriously failing because std changed up their implementation of tempdir).

Stebalien commented 1 year ago

It's documented:

On Unix, returns the value of the TMPDIR environment variable if it is set, otherwise for non-Android it returns /tmp.

(android is a unix platform)

GregoryConrad commented 1 year ago

@Stebalien Sorry; I misinterpreted this:

On Unix, returns the value of the TMPDIR environment variable if it is set, otherwise for non-Android it returns /tmp. On Android, since there is no global temporary folder (it is usually allocated per-app), it returns /data/local/tmp.

I took that second sentence out of context; I thought it meant it will always return /data/local/tmp, not just when TMPDIR is unset.

std::env::set_var("TMPDIR", ...) did the trick as you suggested! Thank you! Closing this issue as that fixes it.