Hacker News new | past | comments | ask | show | jobs | submit login

What "gotchas" or caveats are you expecting from `is_uploaded_file` or `move_uploaded_file`?

Any use of `exec` should be considered dangerous, when there's native language/stdlib support for achieving the same thing.

Edit: corrected function name.




`is_uploaded_file` and `move_uploaded_file` are the band-aid API meant for broken code that may be mistakenly working with some random local files instead of uploaded ones.

This is akin to using "if (3 == x)" notation in C in a fear of accidentally writing "if (x = 3)".

For this particular case the use of both `rename` and `exec` would've been perfectly fine.

> Any use of `exec` should be considered dangerous

By whom? It may be a red flag if used by an inexperienced programmer or if it appears in an otherwise messy code base. But if I know what I'm doing, then it's just another API that's perfectly OK when used with due care.


Part of knowing what you're doing involves knowing the standard library of the language you've chosen to use. There might be (very rare) times you need to use exec in a PHP application - a file upload script is not one of them.


> `is_uploaded_file` and `move_uploaded_file` are the band-aid

It's not a band-aid. When a file is uploaded (see RFC 1867) via HTTP POST, PHP internally allocates a hash table that tracks which files on the filesystem have been uploaded. When the process terminates, it uses that hashtable to remove any remaining uploaded files in the tmp space.

move_uploaded_file() confirms that the filename was indeed created as a result of an upload, and additionally removes that entry from the hash table.


> move_uploaded_file() confirms that the filename was indeed created as a result of an upload, and additionally removes that entry from the hash table.

In other words, move_uploaded_file() protects against moving a non-uploaded file or moving one more than once.

So it's for an upload processing code that, at some point, is becoming unsure of what the heck it is doing.

It is a band-aid.


> In other words, move_uploaded_file() protects against moving a non-uploaded file or moving one more than once.

Sure.

> So it's for an upload processing code that, at some point, is becoming unsure of what the heck it is doing.

How do you get to this conclusion from the above?

> It is a band-aid.

Nope. Uploaded file checking would seem to be necessary, though rather than requiring the developer to implement it, there's some lang-sec value in php doing the check automatically with any $_FILES references and instead allowing the developer to suppress it. That's an area where implementation can be refined, but it doesn't make the current solution a band-aid by any stretch.

Edit: as an example, I'd much rather see the example given in php docs actually implemented as part of the framework rather than delegated to the developer. But aside from just dumping all uploaded files in one quarantined location, this solution is a functional pattern. https://www.php.net/manual/en/function.is-uploaded-file.php

(I'm not a php dev, and docs on the relevant functions date back to 2003, so for all I know, this has been rectified in the intervening 17 years)


[flagged]


> You're creating a memory leak

A resource leak. That's assuming that you are referring to temp files not getting removed once the POST handler exits... which is not how it works.

https://www.php.net/manual/en/features.file-upload.post-meth...

    The file will be deleted from the temporary directory 
    at the end of the request if it has not been moved 
    away or renamed. 
> the internal rfc1867 uploaded files hash table.

This RFC does not talk about life cycle management of uploaded files at all, leave alone such specifics as hash tables.


[flagged]


Yikes, please don't go on tilt like this on HN, regardless of how frustrating a conversation has become.

https://news.ycombinator.com/newsguidelines.html


[flagged]


Consider me impressed, you just posted here common sense and basic knowledge and yet triggered them hard. (Two comments in a row even.)

Is there a way to call Dang to have a look at those offensive comments? :-)


You can flag a comment by clicking on its timestamp to go to its page, then click 'flag' at the top.

In egregious cases, email us at hn@ycombinator.com.


By that logic, I guess you are fine with using `exec` to run Python code to rename the file? It's "just another API".


`exec` is and should be disabled for as many hosts as possible.

The OPs code might be totally fine, but that other script laying there since 1999 might not be. Or a third party library, or else.




Consider applying for YC's Spring batch! Applications are open till Feb 11.

Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: