Make WordPress Core

Opened 10 years ago

Closed 9 years ago

#5768 closed defect (bug) (invalid)

Warning: closedir(): supplied argument is not a valid Directory resource in /wp-includes/theme.php on line 166

Reported by: dustinsilva Owned by:
Milestone: Priority: low
Severity: minor Version: 2.3.2
Component: Optimization Keywords: dev-feedback
Focuses: Cc:


Hello all! I recently upgraded to 2.3.2. I have 'warnings' enabled in apache and I get this warning while trying to change themes: 'Warning: closedir(): supplied argument is not a valid Directory resource in /wp-includes/theme.php on line 166'

Basically line 166 of file 'theme.php' tries to closedir($theme_dir); and I checked the value of $theme_dir at failure, and $theme_dir = ;

You wordpress Devs need to add 'if( is_dir($theme_dir))' to line 166, directly above @closedir(theme_dir)... incase $theme_dir isn't really a readable directory.

or do whatever you think is necessary, but this would be handy addition to this file.

Thank, I hope this is clear.

Not sure what 'Milestone' is in ticket submission form.

Attachments (2)

5768.diff (1.4 KB) - added by DD32 10 years ago.
5768.2.diff (1.5 KB) - added by DD32 10 years ago.
+ ignore cvs

Download all attachments as: .zip

Change History (8)

#1 @markjaquith
10 years ago

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

(In [6744]) Make sure the theme_dir is a dir before we attempt to closedir(). fixes #5768

#2 @DD32
10 years ago

Shouldnt've the change been applied here: http://trac.wordpress.org/browser/trunk/wp-includes/theme.php#L135

It should fail to open the theme directory and then return false if its not a valid directory.. Perhaps a is_dir() should've been added there instead?

While i'm on that piece of code: http://trac.wordpress.org/browser/trunk/wp-includes/theme.php#L159

|| $theme_dir == 'CVS' )

Shouldn't that be checking for subversion stuff instead now-days?(if even needed)

#3 @DD32
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Ignore that "Shouldnt've the change been applied here:" line, I mis-read the source code.

But i should say i'm rather confused by the source-code. (And have made some points somewhere throughout this ramble, So just loook at the patch, and then here for why i've changed what i have)

Line 135: The directory is attempted to be opened.

	$themes_dir = @ opendir($theme_root);

Line 136-7: If the directory cannot be opened, $themes_dir = false, function should return.

	if ( !$themes_dir )
		return false;

Line 139-177: Loops through the $themes_dir folder, Sets $theme_dir to False when its finished.

Line 178: WordPress attempts to close "$theme_dir", $theme_dir is now set to false, As it was the used in the 139-177 loop for the filename.

Its this last line which had me confused.

is_dir($theme_dir) should NEVER be true, as when the loop above it finishes, it MUST be false.

I think line 178 was supposed to close the folder handle for that folder, And if we look back to line 135, thats $themes_dir, While this code attempts to close $theme_dir;

So, Line 178 should read:

	@closedir( $themes_dir );

Correct, or have i missed something?

Also, If we move furthur onto line 180-181:

	if ( !$themes_dir || !$theme_files )
		return $themes;

Previously $themes_dir was true as it was a open file handle.. And it'll never be false, as up on line 136, it returns if the themes_dir is false, and its not modified by the main loop at all. (And with the change given, it'll be false as it'll be a closed file handle)

The CVS Stuff:

Probably should've mentioned that in a different ticket, But it was a view on the code in the same area. I mentioned it as 'CVS' is the folder which the CVS setup uses to store info in, Whilst Subversion uses '.svn' instead.

If we look at line 158-160: (note: Also 139-141 gets the same treatment)

if ( is_dir( $subdir . '/' . $theme_dir) && is_readable($subdir . '/' . $theme_dir) ) {
    if ( $theme_dir{0} == '.' )

$theme_dir{0} == '.' will match '.', '..', '.svn' and anything else hidden. And its also checks that *after* its checked its a directory & readable, both checks which can be skipped by moving the code up a line.

CVS: I can see that it might be a issue by removing that however now that i've written this load of BS out, as those who check their themes in/out of a CVS branch might hit issues..

I've attached a patch with the changes (And i'll add one which ignores CVS directories too) - I apologise for the long writeup on the code, It was needed for me to understand the entire lot, as well as explaining where my mind was going with the code.

10 years ago

10 years ago

+ ignore cvs

#4 @lloydbudd
10 years ago

  • Priority changed from normal to low
  • Severity changed from blocker to minor

#5 @DD32
9 years ago

  • Keywords dev-feedback added; closedir warning not valid directory theme.php removed

Traction? Close as wontfix? Remove ancient CVS references?

#6 @Denis-de-Bernardy
9 years ago

  • Milestone 2.9 deleted
  • Resolution set to invalid
  • Status changed from reopened to closed

let's close as invalid, as it's probably fixed in #7875

Note: See TracTickets for help on using tickets.