53244 - [RFE] Recursive directory creation in Unix installer

Status: VERIFIED FIXED

(SeaMonkey :: Installer, enhancement, P1)

Description

[Grace Bush]

Installer does not allow 2 (or more) levels of directory creation during 
installation process

Linux installer, unlike Win Installer, only allows user to enter one level of 
directory creation.  ie you can select /u/user/test2/test1 (assuming 
/u/user/exists) - installer will prompt to create but will only create test2 and 
not next level test1

Comment 1

[Sean Su]

rassigning to Samir.

Comment 2

[Samir Gehani]

This is by design, i.e., non-recursive dir creation.  Grace, does the installer
display a different dir than is created (e.g., does it display
/u/user/test2/test1/ when in fact it only created /u/user/test2 or does the
display not lie)?

At any rate, we can fix this to do recursive dir creation; next release though.

Comment 3

[Grace Bush]

Installer displays path I set, asks me if I want to create, when I say yes- it 
gives me error (-624)(only problem is it does not remove this dialog)
It does not allow me to install so I change to one level of creation and I am 
able to install successfully.

Comment 4

[Samir Gehani]

I see the problem.  When you click OK on the error dialog it doesn't go away but
the OK button disappears.  This is poor UE that should get fixed next release too.

Comment 5

[André Dahlqvist]

*** Bug 97897 has been marked as a duplicate of this bug. ***

Comment 6

[Daniel Veditz [:dveditz]]

nominating this simple change

Comment 7

[Syd Logan]

sounds like fun

Comment 8

[Denis Antrushin]

Attachment: recursive mkdir

Comment 9

[Denis Antrushin]

Something like attachment 54052 [details] [diff] [review] ?

Comment 10

[Syd Logan]

+    memcpy(path, gCtx->opt->mDestination, (pathLen > PATH_MAX) ? PATH_MAX :
pathLen);
+    path[pathLen] = '/';  // for uniform handling


So, you copy PATHMAX if pathLen is greater than PATHMAX. At first glance, that
appears ok. But, let pathLen == PATHMAX * 2. You will then be setting
path[PATHMAX * 2] to '/', exceeding the array bounds because you are using
pathLen in that second statement.

How about :

if ( pathLen > MAXPATH) 
     pathLen = MAXPATH;
memcpy( path, gCtx->opt->mDestination, pathLen );
path[pathLen] = '/';
path[pathLen + 1] = '\0';

this is simpler, eliminates the potential buffer overflow. Note also I added the
NULL ('\0') that you forgot to add to the end of the buffer (can't assume the
memory is filled with NULLs).

I notice you check for errs but do nothing with them. What happens if you fail a
mkdir?



 

Comment 11

[Denis Antrushin]

> You will then be setting path[PATHMAX * 2] to '/', exceeding 
> the array bounds because you are using pathLen in that second 
> statement.

Oops, sorry, missed that

> path[pathLen + 1] = '\0';
> NULL ('\0') that you forgot to add to the end of the buffer (can't assume the
> memory is filled with NULLs).

Actually, I don't need terminate array with '\0', because I'll replace '/' with 
'\0' inside the loop and I don't rely on '\0' in loop condition 
( for (int i = 1; !err && i <= pathLen; i++) )

> I notice you check for errs but do nothing with them. What happens if you fail a
> mkdir?

Those lines didn't fit into diff, look a copule of lines below:

    if (err != 0)
    {
        ErrorHandler(E_MKDIR_FAIL);
        return;
    }



 

Comment 12

[Denis Antrushin]

Attachment: fixed buffer overflow

Comment 13

[Denis Antrushin]

Attachment: fix case typos in prev patch

Comment 14

[Syd Logan]

Comment on attachment 56242 [details] [diff] [review]
fix case typos in prev patch

looks ok, thanks for the patch

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 56242 [details] [diff] [review]
fix case typos in prev patch

sr=dveditz

Comment 16

[Syd Logan]

fixed

Comment 17

[Grace Bush]

verified in build 2002020809

Comment 18

[Brant Gurganus]

[RFE] is deprecated in favor of severity: enhancement.  They have the same meaning.