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

Use a tempfile for file fields #7

Open
Vypo opened this issue Apr 2, 2020 · 4 comments
Open

Use a tempfile for file fields #7

Vypo opened this issue Apr 2, 2020 · 4 comments

Comments

@Vypo
Copy link

Vypo commented Apr 2, 2020

If a file is uploaded by a user, and stored in the file system, it hangs around forever. If the program exits, crashes, or even if the developer ignores the file, it will clutter the temp directory.

I think using tempfile() (or if there are too many issues with unnamed files, NamedTempFile) would deal with the issue effectively.

@magiclen
Copy link
Owner

magiclen commented Apr 3, 2020

Thanks for your advice.

If the developer ignores files that are stored in the file system after calling the parse method of a MultipartFormData instance, those files should be deleted automatically when the MultipartFormData instance is dropped. (unless the developer changes the MultipartFormData instance)

impl Drop for MultipartFormData {
    #[inline]
    fn drop(&mut self) {
        let files = &self.files;

        for field in files.values() {
            match field {
                FileField::Single(f) => {
                    try_delete(&f.path);
                }
                FileField::Multiple(vf) => {
                    for f in vf {
                        try_delete(&f.path);
                    }
                }
            }
        }
    }
}

Therefore, I think using NamedTempFile would make no difference.

The tempfile() function is using the O_TMPFILE | O_EXCL flags on Linux or the FILE_ATTRIBUTE_TEMPORARY | FILE_FLAG_DELETE_ON_CLOSE flags on Windows to create a temporary file, which should prevent leaking when the program exits or crashes. But I don't know how to transform this kind of a temporary file to a regular file......

@Vypo
Copy link
Author

Vypo commented Apr 3, 2020

Thanks for getting back to me! I didn't realize there was already some functionality for this. I noticed a couple files hanging around, and naively searched for Drop in multipart_form_data_field.rs and nowhere else.

Would it then make sense to add a lifetime to SingleFileField, to show that the path is only valid as long as the MultipartFormData is alive?


The manpage for open(2) has a nice example of using linkat to assign a name to a file descriptor, though I think that's probably way out of scope of a multipart upload crate for Rocket.

char path[PATH_MAX];
fd = open("/path/to/dir", O_TMPFILE | O_RDWR,
                      S_IRUSR | S_IWUSR);

/* File I/O on 'fd'... */

linkat(fd, NULL, AT_FDCWD, "/path/for/file", AT_EMPTY_PATH);

/* If the caller doesn't have the CAP_DAC_READ_SEARCH
 capability (needed to use AT_EMPTY_PATH with linkat(2)),
 and there is a proc(5) filesystem mounted, then the
 linkat(2) call above can be replaced with:

snprintf(path, PATH_MAX,  "/proc/self/fd/%d", fd);
linkat(AT_FDCWD, path, AT_FDCWD, "/path/for/file",
                      AT_SYMLINK_FOLLOW);
*/

@magiclen
Copy link
Owner

magiclen commented Apr 4, 2020

Is there any benefit to add a lifetime which is not necessary?

If linkat can do the job on Linux, how about Windows and other operating systems?

@Vypo
Copy link
Author

Vypo commented Apr 8, 2020

Is there any benefit to add a lifetime which is not necessary?

I would say so. It clearly indicates to the developer that the SingleFileField is only valid for a limited time. Though I may not be the most astute developer, I clearly missed that while reading the docs, so there'd be at least one less person bitten by it.

If linkat can do the job on Linux, how about Windows and other operating systems?

I think you can use CreateHardLink on windows. I haven't tested it though.

Other unix implementations create the file normally, then unlink it immediately. I don't think there's a way to relink the file.

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

No branches or pull requests

2 participants