Project

General

Profile

Bug #7454

rmdirr() may fail and still return return true.

Added by Torkil Zachariassen over 6 years ago. Updated over 6 years ago.

Status:
Needs more info
Priority:
Normal
Assignee:
-
Category:
System
Target version:
Start date:
18/12/2014
Due date:
% Done:

0%

Estimated time:
Complexity:
Normal
SCRUM pts - complexity:
?

Description

Cleaning up in /archive using /main/admin/archive_cleanup.php may seem to work, while the actual cleaning has problems as rmdirr() lacks proper error handling when going recursive.

rmdirr() in main_api.lib.php has a flaw in error handling as it may fail, but still return true.
The return value should be TRUE on success and FALSE on failure.

Here is a patch

--- main_api.lib.php.000        2014-11-20 19:56:00.776123612 +0000
+++ main_api.lib.php    2014-12-18 14:55:54.459171298 +0000
@@ -4205,7 +4209,12 @@
             }

             // Recurse.
-            rmdirr("$dirname/$entry");
+            $res=rmdirr("$dirname/$entry");
+            if ($res === false ) {
+              error_log(__FILE__ . ' line ' . __LINE__ . ": rmdirr() failed on $dirname/$entry");
+              return $res;
+            }
+
         }
     }

Related issues

Related to Chamilo LMS - Bug #7455: Improve error message when archive cleanup failsBug resolved18/12/2014

Actions

Associated revisions

Revision 33088845 (diff)
Added by Julio over 6 years ago

Adding strict mode see #7454

History

#1

Updated by Julio Montoya over 6 years ago

  • Category set to System
  • Target version set to 2.0

The "rmdirr" function already contains a error log if it fails here:

 if (is_file($dirname) || is_link($dirname)) {
        $res = unlink($dirname);
        if ($res === false) {
            error_log(__FILE__.' line '.__LINE__.': '.((bool)ini_get('track_errors') ? $php_errormsg : 'Error not recorded because track_errors is off in your php.ini'), 0);
        }
        return $res;
    }

No need to add another one.

But I agree that we need a better handling error using probably exceptions.
I'm moving this to v2.0

#3

Updated by Julio Montoya over 6 years ago

  • Status changed from New to Needs more info

Also available in: Atom PDF