developer.jelix.org is not used any more and exists only for history. Post new tickets on the Github account.
developer.jelix.org n'est plus utilisée, et existe uniquement pour son historique. Postez les nouveaux tickets sur le compte github.

Opened 11 years ago

Closed 11 years ago

#840 closed bug (fixed)

bad deleted directory with jelix scripts

Reported by: foxmask Owned by: laurentj
Priority: highest Milestone: Jelix 1.1.1
Component: jelix-scripts Version: 1.1
Severity: blocker Keywords:
Cc: Blocked By:
Blocking: Documentation needed: no
Hosting Provider: Php version:

Description

Hi,

In jelix-scripts.init.php, when JELIX_APP_TEMP_PATH is not well defined (eg with an unexisting path), the php function realpath, return the root of the Filesystem.

When Jelix.php is called (eg php jelix.php --myappp createmodule foobar) , it remove the content of the temp dir but as this one is the "root" ; this clear the complet filesystem.

thus, after the include in jelix.php of this line

define ('JELIX_APP_TEMP_PATH',    realpath(JELIX_APP_PATH.'../temp/havefnubb/').DIRECTORY_SEPARATOR);

may be we should test if the value of JELIX_APP_TEMP_PATH is not "/" nor "\".

Regards.

Attachments (4)

ticket-840.patch (1.2 KB) - added by laurentj 11 years ago.
patch v1
840.diff (4.8 KB) - added by Julien 11 years ago.
840.2.diff (9.8 KB) - added by Julien 11 years ago.
updated patch with corrected indentation
840-jFile-removeDir-cannot-delete-root-dir.diff (3.3 KB) - added by Julien 11 years ago.
another approach: don't allow jFile::removeDir to remove the FS root dir

Download all attachments as: .zip

Change History (16)

comment:1 Changed 11 years ago by laurentj

  • Milestone set to Jelix 1.1.1
  • Owner set to laurentj
  • Priority changed from normal to highest
  • Severity changed from normal to blocker
  • Status changed from new to assigned
  • Summary changed from Jelix.php installer script to bad deleted directory with jelix scripts
  • Type changed from task to bug

comment:2 Changed 11 years ago by foxmask

May be it's stupid (as the script jelix-scripts.init.php is generated by jelix.php) but in jelix-scripts.init.php if we check that the directory JELIX_APP_PATH.'../temp/havefnubb/' exist before using realpath and defining the JELIX_APP_TEMP_PATH constant, this will avoid to meet some weirds behaviors when a coder change the script (jelix-scripts.init.php) by hand, for example, after making an update from an older version than 1.1 final.

with something like

if (file_exist(JELIX_APP_PATH.'../temp/havefnubb/') define ('JELIX_APP_TEMP_PATH',realpath(JELIX_APP_PATH.'../temp/havefnubb/'.DIRECTORY_SEPARATOR);

then in jelix.php ; JELIX_APP_TEMP_PATH will not be defined if the path is wrong and then the user will be noticed that this error occurs.

Changed 11 years ago by laurentj

patch v1

comment:3 Changed 11 years ago by laurentj

  • review set to review?

foxmask : does this patch fix the bug ? (I haven't a test environment here to test it)

comment:4 Changed 11 years ago by foxmask

  • review changed from review? to review+

in jelix-scripts-init.php i did :

define ('JELIX_APP_TEMP_PATH',    realpath(JELIX_APP_PATH.'../temp/toto-jelixxxxx-scripts/').DIRECTORY_SEPARATOR);

which path does not exist :

then i enter on ms-command :

E:\EasyPHP3\www2\lib\jelix-scripts>e:\EasyPHP3\php\php jelix --toto createmodule
 tata
#PHP=`which php`
exec `which php` -d output_buffering=1 $0 $@
Error: bad path in JELIX_APP_TEMP_PATH, it is equals to '\' !!
       Jelix cannot clear the content of the temp directory.
       Correct the path in JELIX_APP_TEMP_PATH or create the directory you
       indicated into JELIX_APP_TEMP_PATH.

this validates the patch as the JELIX_APP_TEMP_PATH is equal to the DIRECTORY_SEPARATOR.

comment:5 Changed 11 years ago by laurentj

ok

just a thing about the use of jelix-scripts: run "jelix" (on unix system), or "php jelix.php", but not "php jelix" ;-)

comment:6 Changed 11 years ago by Julien

I think I found other parts of the code which may have the same problem. Here's an updated patch.

Maybe we should also forbid file system root deletion directly inside the jFile::removeDir method ?

Changed 11 years ago by Julien

comment:7 Changed 11 years ago by Julien

  • review changed from review+ to review?

comment:8 Changed 11 years ago by laurentj

  • review changed from review? to review+

why not. But carefull, it seems that your indentations are wrong...

Changed 11 years ago by Julien

updated patch with corrected indentation

comment:9 Changed 11 years ago by Julien

Here's an updated patch, that corrects indentation, which was sometimes 3 chars instead of 4 in jelix.php; so komodo didn't convert my tabs to 4 spaces....

Changed 11 years ago by Julien

another approach: don't allow jFile::removeDir to remove the FS root dir

comment:10 Changed 11 years ago by Julien

  • review changed from review+ to review?

They are now 2 different patches to solve this issue. Which one do you prefer ?

I think the patch for jFile is necessary, as it will cover any case.

I wonder if the first approach is still relevant (at least it's useful because it tells the users what is wrong).

Should we commit both ?

comment:11 Changed 11 years ago by laurentj

  • review changed from review? to review+

ok, commit them into 1.1.x + trunk

thanks

comment:12 Changed 11 years ago by laurentj

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

landed. r1318

Note: See TracTickets for help on using tickets.