Make WordPress Core

Opened 14 years ago

Closed 14 years ago

#12232 closed defect (bug) (fixed)

Invalid Archive. Theme Install Failed. Caused by ftp_rawlist using -a option

Reported by: frankindelicato's profile frankindelicato Owned by: dd32's profile dd32
Milestone: 3.0 Priority: normal
Severity: major Version: 2.9.1
Component: Filesystem API Keywords: ftp_rawlist
Focuses: Cc:

Description

Downloading any theme gives me this error:
Unpacking the package.
Installing the theme.
Incompatible Archive
Theme Install Failed.

I am running Mac OS X 10.6 using both MAMP and Apple Servers. The problem was found in the Worpress-MU code but the error is also in Wordpress.

The problem was traced to the @ftp_rawlist call in the function dirlist() in the class file class-wp-filesystem-ftpext.php. The @ftp_rawlist call preforms an ftp LIST command and is supposed to return a listing of a directories contents, including any folders in the directory. It was returning a boolean false instead of an array of file/folder listings.

It is fixed by removing the -a in the ftp_rawlist call.

The old line of code reads:
$list = @ftp_rawlist($this->link, '-a ' . $path, false);

The new line of code should read:
$list = @ftp_rawlist($this->link, $path, false);

Removing the -a does not effect the functionality of the dirlist() function because a few lines further down in the code the "." and ".." files/folders are ignored anyway.

The issue was discussed in the wordpress-mu forum, in the topic:
href=http://mu.wordpress.org/forums/topic/16777

This bug is also related to the ticket: #10060

Change History (18)

#1 @frankindelicato
14 years ago

  • Keywords ftp_rawlist added
  • Version set to 2.9.1

#2 @nacin
14 years ago

  • Milestone changed from Unassigned to 3.0
  • Owner set to dd32
  • Status changed from new to assigned

#3 @dd32
14 years ago

I'm unsure how this error ("Incompatible Archive") is being thrown in all honesty.

That particular error should only be thrown in the event that the Zip file downloaded to the filesystem cannot be opened (This is seperate from the filesystem access): http://core.trac.wordpress.org/browser/trunk/wp-admin/includes/file.php#L550

I dont have access to a Mac, or a Rackspace account to verify.

I will take a look and see if the -a flag is still needed, its probably extra cruft now.

Theres another report of this here: #12230

#4 follow-up: @dd32
14 years ago

May have jumped the gun there, I see you're reporting this in 2.9.1.. In which case, I'm still confused why that error is thrown, but its not related to ZipArchive :)

#5 in reply to: ↑ 4 @frankindelicato
14 years ago

Replying to dd32:

May have jumped the gun there, I see you're reporting this in 2.9.1.. In which case, I'm still confused why that error is thrown, but its not related to ZipArchive :)

Yes, it does not seem to be a zipping problem, In fact, the .zip file gets downloaded and unzipped properly into a temporary folder named "the-new-theme-name.tmp" but then when the system goes to move the unzipped files it tries to access the temporary folder to get the files/folders contents. That's when the $list=@ftp_rawlist function fails. The function does not return an error (any error that might be produced is not tested for anyway), so the system just runs along as though it did receive the array of directory contents until the error is thrown somewhere else, do not remember where it was thrown but it was much further along in the code....

#6 @dd32
14 years ago

My error there.. Seems that "Incompatible Archive" is also the error for the source directory having 0 files.. Which is why its rawlist() causing the problems..

Removing -a will fix this, But also means that any "hidden" directories will not be listed, which can be a problem when people want to overwrite a SVN checkout with the plugin upgrader for example.

I think it'd also have an effect on the return data, Given that we dont always check to see if a folder exists before listing its contents (Saves on the number of FTP calls needed), we'd probably have to expand the code related to ftp_rawlist returning nothing.

Not having access to a Mac doesnt help me debug/work around this issue.. Macs are pretty common amongst some IT crowds, so i'm surprised this hasnt occured before if its a widespread issue.

I'm reducing the Severity of this, since its not a regression from a previous version, and it seemingly only affects a smaller group of hosts.

#7 @frankindelicato
14 years ago

I am not sure as I am new to wordpress but the hidden directories were not being used anyway. After the rawlist call dirlist() builds an array to return to its caller and when it builds the array it ignores the hidden files. See below:

$dirlist = array();
foreach ( $list as $k => $v ) {

$entry = $this->parselisting($v);
if ( empty($entry) )

continue;

if ( '.' == $entryname?
'..' == $entryname? ) THIS SKIPS THE HIDDEN FILES

continue;

if ( ! $include_hidden && '.' == $entryname?[0] )

continue;

if ( $limit_file && $entryname? != $limit_file)

continue;

$dirlist[ $entryname? ] = $entry;

}

So, in addition to fixing up the problem with returning nothing, (actually dirlist() returns a boolean false instead of an empty array and this causes a PHP Warning to be issued when the boolean is used by a following array_keys call that is trying to use what it thinks is a returned array) I guess you would have to test for how the hidden files are used when people want to overwrite with a SVN checkout. I need to study PHP and Wordpress more so if you like (and I have the Mac) I can pursue the issues. You would have to point me in the right direction though as I do not have the SVN stuff setup yet (pointing me to some SVN knowledge would help) and another other suggestions you may have...

#8 @eoinomurchu
14 years ago

I'm currently experiencing this problem also. I'm fairly new to wordpress/php so shoot me if you think this is silly.

I modified dirlist from

		$list = @ftp_rawlist($this->link, '-a ' . $path, false);

		if ( empty($list) ) // Empty array = non-existent folder (real folder will show . at least)
			return false;

to

		$chdir = @ftp_chdir($this->link, $path);
		$list = @ftp_rawlist($this->link, '-a');

		if ( empty($list) || !$chdir )
			return false;

ftp_rawlist doesn't seem too happy when you stick '-a' to the front of the path, but is perfectly happy if you chroot there first and then call LIST -a.

I'm not sure why this is the case, but it seemed to solve the problem for me.

Also, I downloaded the latest nightly build, that is what I am working on. Not sure what version number that is. As I said, I'm new to all this :)

#9 @frankindelicato
14 years ago

Yes, I tried chdir also but found it caused path errors later in the code. I fixed that by saving the current directory, then used chdir, and then restored the current directory. But I abandoned that approach and just took out the -a because if you look further down in the dirlist() function they skip the hidden directories anyway and they are never used.

#10 @eoinomurchu
14 years ago

I don't think that code will skip hidden files in this case.

if ( '.' == $entryname? '..' == $entryname? ) // THIS SKIPS THE HIDDEN FILES
    continue;

The above section of code will skip over the special directory listings for current dir and parent dir. This is commented wrong. Since there are more hidden files than just these two.

if ( ! $include_hidden && '.' == $entryname?[0]  )
    continue;

And this would skip the remaining hidden files (files starting with a .) if $include_hidden is false, which it's not. It's set by default to true in the function/method definition, and a different value is not passed in this case.

I thought about saving the current dir and resetting it after the operation, but if I remember correctly, when I tested the value of PWD before the operation, it returned an empty string.

#11 @eoinomurchu
14 years ago

That said, my last point about PWD is incorrect, I was having some issues further on in the code. The following corrected it, and I don't seem to be having any problems.

So in wp-admin/includes/class-wp-filesystem-ftpext.php

replace

	$list = @ftp_rawlist($this->link, '-a ' . $path, false);

	if ( empty($list) ) // Empty array = non-existent folder (real folder will show . at least)
		return false;

with

	$pwd = @ftp_pwd();
	$chdir = @ftp_chdir($this->link, $path);
	$list = @ftp_rawlist($this->link, '-a');
	@ftp_chdir($this->link, $pwd);

	if ( empty($list) || !$chdir ) // Empty array = non-existent folder real folder will show . at least)
		return false;

This is a hack/workaround. It's not very pretty, but it does seem to fix the error.

#12 @dd32
14 years ago

This is a hack/workaround. It's not very pretty, but it does seem to fix the error.

I agree that its not that pretty, but i'm tempted to go with that for 3.0 to fix the issue for those having it, whilst not affecting the functioning of the function for now.

#13 @dd32
14 years ago

  • Component changed from General to Filesystem
  • Severity changed from blocker to major

Reducing the severity on this ticket, Its not a regression from a previous release, and therefor, isn't a blocker.

I'm going to commit the work around, With the notion, that if anyone at all has an issue that could be related to this during the beta testing, it'll be reverted and looked at for 3.1 instead.

#14 @dd32
14 years ago

(In [13850]) Attempt to work around Mac FTP Server security implications of using '-a' in FTP. Props eoinomurchu. See #12232

#15 @dd32
14 years ago

Seems i forgot to include the !$chdir here, is causing a problem: #12057

#16 @dd32
14 years ago

(In [14120]) Return false on WP_Filesystem_FTPext::dirlist() for non-existant folders. See #12232. See #12057

#17 @dd32
14 years ago

I have not seen any issues which I can blame on this changeset since the last bug 5 weeks back, I'll leave this open until RC time, If nothing comes up in RC1, I'll assume that this hasnt broken anything & can close it then.

#18 @nacin
14 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.