107803 - Not enough disk space in for /tmp

Status: VERIFIED FIXED

(SeaMonkey :: Installer, defect)

Description

[sairuh (rarely reading bugmail)]

using the stubs installer for the 2001.10.31.08 build [commercial, on linux
rh6.2], the xpi's are downloaded, but the installation fails.

i get the following error while extracting libmozjs.so: "Fatal error [-616]:
Extraction of XPCOM failed."

Comment 1

[sairuh (rarely reading bugmail)]

eek, i tried using the non-installer blob, but when i issue 'tar zxvf
[blobname]' i'm unable to completely decompress it:

gzip: stdin: unexpected end of file
tar: Unexpected EOF in archive
tar: Child returned status 1
tar: Error exit delayed from previous errors
fwiw, it fails after decompressing package/chrome/comm.jar...

grace and curt will prolly come over to see if there's something fishy with my
machine. it might be a bug, but then again it might be something odd that has a
workaround...

Comment 2

[sairuh (rarely reading bugmail)]

hrm, wonder if my dir permissions have changed...

Comment 3

[sairuh (rarely reading bugmail)]

this might be due to lack of disk space, d'oh!

testing now...

Comment 4

[sairuh (rarely reading bugmail)]

hrm...actually, the /export partition on which i usually install is only 68%
full. the / partition is at 100%, but i guess i don't understand how that might
be causing this... other suggestions? i'll try removing some stuff off of / ...

Comment 5

[sairuh (rarely reading bugmail)]

okay, i removed a few things from / , although that still reports 100% used...
then i also removed the install directory, recreated it...then was successful at
installing using the stubs.

closing out as wfm.

Comment 6

[Daniel Veditz [:dveditz]]

The installer unpacks things into /tmp. I thought the installer accounted for
that required space when /tmp is on a different volume than the install
directory, but maybe it's only windows that does that.

Comment 7

[Curt Patrick (gone)]

Reopening based on Dan's comments and changing the summary to reflect the
problem that we really care about.  Seems that we should spot this problem and
give the user some clue about what it going on.

Comment 8

[Curt Patrick (gone)]

I'll take this bug

Comment 9

[sairuh (rarely reading bugmail)]

bumping down sev, since i was able to find a workaround [threw out some big
items from /tmp, even tho' / remained at 100%, and removing the previous install
dir].

Comment 10

[sairuh (rarely reading bugmail)]

curt, i wonder if this might be dependent on bug 55690?

Comment 11

[Syd Logan]

a comment

Comment 12

[Daniel Veditz [:dveditz]]

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

Comment 13

[Syd Logan]

Attachment: Improve how we do temp directory creation and removal

A couple of problems:

We should let the system give us a temporary name, this is preferred in unix.
For one, /tmp might not exist. Or, the user may prefer a different temp
directory. tempnam() is designed to give an appropriate unique name, and take
into account the TMP and TMPDIR env variables (which we ignored). In many cases
when the user fails because of minimal space in /tmp, he or she will have set
TMPDIR in their sh init file to specify a place that has more space. Also, if
the user fails, we can suggest they change TMPDIR to some other location and
rerun the installer. By hardcoding /tmp, they have to clear up /tmp or we will
fail, there is no other workaround.

Another problem I fixed is we never remove the temporary directory in the
engine destructor (we do free the memory allocated to the member variable that
holds the path, for what that is worth). So, added that code in as well.

Comment 14

[Syd Logan]

Regarding the above patch, someone will point out potential security
implications using tempnam() (race conditions). These issues should only apply
to file creation, here we are creating a directory, not a file. I scanned the
appropriate bulletins and this is usage of tempnam() is not a problem.

Comment 15

[Samir Gehani]

Comment on attachment 67237 [details] [diff] [review]
Improve how we do temp directory creation and removal

r=sgehani

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 67237 [details] [diff] [review]
Improve how we do temp directory creation and removal

>+    if ( mTmp != (char *) NULL ) {

That's pretty ugly, especially the cast. Couldn't you "if ( mTmp )"?
If you absolutely must "if ( mTmp != 0 )", but the NULL #define is
discouraged in C++

ditto with if (buf)

>+    buf = tempnam( (const char *) NULL, "xpi" );
> 
>-    mTmp = (char *) malloc( (strlen(buf) * sizeof(char)) + 1 );
>+    if ( buf != (char *) NULL ) 
>+      mTmp = (char *) malloc( strlen(buf) + 1 );
>     if (!mTmp) return E_MEM;
>-
>     sprintf(mTmp, "%s", buf);
>-    mkdir(mTmp, 0755);
>-
>+    err = mkdir(mTmp, 0755);
>+    if ( err == -1 )
>+      err = E_DIR_CREATE;
>     return err;
> }

This routine leaks buf.

Comment 17

[Syd Logan]

Casting to (char *) NULL is very standard C. While NULL is almost always == 0,
it is not guaranteed. Man page for malloc() states NULL is returned on failure
(and makes no guarantee it is 0). 

Thanks for catching the leak, I'll put up a new patch.

Comment 18

[Syd Logan]

Attachment: New patch, fix leak in first patch

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 68031 [details] [diff] [review]
New patch, fix leak in first patch

> nsXIEngine::MakeUniqueTmpDir()
> {
>     int err = OK;
>     int i;
>-    char buf[MAXPATHLEN];
>-    struct stat dummy;
>+    char *buf; 
>     mTmp = NULL;
> 
>-    for (i = 0; i < MAX_TMP_DIRS; i++)
>-    {
>-        sprintf(buf, TMP_DIR_TEMPLATE, i);
>-        if (-1 == stat(buf, &dummy))
>-            break; 
>-    }
>+    buf = tempnam( (const char *) NULL, "xpi" );
> 
>-    mTmp = (char *) malloc( (strlen(buf) * sizeof(char)) + 1 );
>+    if ( buf != (char *) NULL ) 
>+      mTmp = (char *) malloc( strlen(buf) + 1 );
>     if (!mTmp) return E_MEM;

You still potentially leak buf if you return early here.

>-
>     sprintf(mTmp, "%s", buf);

Didn't notice this bit of ugliness before. Why not strcpy()?

>-    mkdir(mTmp, 0755);
>-
>+    err = mkdir(mTmp, 0755);
>+    if ( err == -1 )
>+      err = E_DIR_CREATE;
>+    free( buf );
>     return err;
> }

Looking at this again I'm not sure you even need buf anymore.
Couldn't you just mTmp = tempnam() now and be done with it?

Comment 20

[Syd Logan]

Attachment: better patch

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 68037 [details] [diff] [review]
better patch

sr=dveditz

Comment 22

[Syd Logan]

fixed

Comment 23

[K'Trina Medina]

Verified code fix