WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#34772 closed defect (bug) (fixed)

get_contents won't unlink tempfile when ftp_fget returns false

Reported by: ruud@… Owned by: dd32
Milestone: 4.5 Priority: normal
Severity: normal Version: 4.3.1
Component: Filesystem API Keywords: has-patch commit
Focuses: Cc:

Description

Hi,

on line 104 of class-wp-filesystem-ftpext.php the get_contents.php functions bails out after an failed ftp-fget attempt.

However because wp_tempnam() creates and touches a fresh new file, more and more files get onto the server.

The unlink call at line 114 isn't used anymore

Attachments (1)

class-wp-filesystem-ftpext.patch (822 bytes) - added by ruud@… 5 years ago.
Crude stab at a patch of some sort

Download all attachments as: .zip

Change History (11)

@ruud@…
5 years ago

Crude stab at a patch of some sort

#1 @ruud@…
5 years ago

  • Keywords has-patch dev-feedback added

#2 @dd32
5 years ago

  • Keywords commit added; dev-feedback removed
  • Milestone changed from Awaiting Review to Future Release

@ruud@joyo Good catch!

The patch looks correct to me too :)

#3 @ruud@…
5 years ago

  • Keywords dev-feedback added

Hi dd32,

Thanks for your quick follow-up.
I was looking into this piece of code because our server was bogged down with trying to seek for a new non existing temp file name.
We ran into errors like:
[23-Nov-2015 07:32:00 UTC] PHP Warning: unlink(/tmp/debug180541.tmp): No such file or directory in ..

but that meant the server already had gone form debug1.tmp all the way up until debug180541.tmp opening files.. etc. to get to a non-existing .tmp file.

While I do like enumeration like this, wouldn't something like using a (part of?) unix timestamp also suffice, and be way more faster?

Such an approach would also alleviate possible future unlink problems as well, but I'm not sure if the current methodology of getting unique filenames has more to it, like it has to be sequential or something.

Thanks,
Ruud

#4 @ruud@…
5 years ago

come to think of it: using timestamps does/may get you into weird stuff when changing server time settings etc. But we could prevent that kind of stuff to do a second round of checks for file existance with the current way, just enumerating to get to a unique filename on top of a timestamp based filename.

#5 @dd32
5 years ago

  • Keywords dev-feedback removed

In 4.4+ wp_tempnam() includes some random data per-call into the filename, which should greatly reduce the chances of it hitting a conflicting filename.

Theoretically servers shouldn't be hitting this code branch too often, if it is, there's likely something else wrong with the server which needs to be investigated.. but that being said, this patch is definitely good, the only reason I didn't commit it immediately was we were about to go RC1 for 4.4.

#6 @dd32
5 years ago

  • Milestone changed from Future Release to 4.5

#7 @dd32
5 years ago

  • Owner set to dd32
  • Resolution set to fixed
  • Status changed from new to closed

In 35777:

Upgrader: FTP: Cleanup temporary files during FTP download failures.
Props ruud@joyo
Fixes #34772

#8 @ruud@…
5 years ago

  • Keywords dev-feedback added

Request removed. I noticed in a later install that more contributors were added to the list :)

Last edited 5 years ago by ruud@… (previous) (diff)

#9 @ruud@…
5 years ago

  • Keywords dev-feedback removed

#10 @ocean90
4 years ago

In 38094:

Filesystem API: Cleanup temporary file when the temporary file couldn't be opened.

Props ruud@joyo.
See #34772.
Fixes #36942, #36943.

Note: See TracTickets for help on using tickets.