magiclen / rocket-multipart-form-data

This crate provides a multipart parser for the Rocket framework.
MIT License
35 stars 14 forks source link

Use a tempfile for file fields #7

Open Vypo opened 4 years ago

Vypo commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.