89436 - Seg fault on install; cannot install

Status: VERIFIED FIXED

(SeaMonkey :: Installer, defect)

Description

[Henry Throop]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux 2.2.16-22 i686; en-US; rv:0.9) Gecko/20010505
BuildID:    20010628

After running the talkback-enabled mozilla-installer, I get hundreds of messages
of the form

> Gtk-CRITICAL **: file gtkprogress.c: line 518 (gtk_progress_set_percentage):
assertion `percentage >= 0 && percentage <= 1.0' failed.
Followed by
> mozilla-installer: line 55:  6661 Segmentation fault     
./mozilla-installer-bin --sync $

Running mozilla-installer again, I get 

> Gtk-CRITICAL **: file gtkwidget.c: line 1507 (gtk_widget_hide): assertion
`widget != NULL' failed.
> mozilla-installer: line 55:  6667 Segmentation fault     
./mozilla-installer-bin --sync $@

Install is straightforward: no proxies, and the files seem to download OK. 
Nothing ever gets copied into my destination distall directory, however.

Reproducible: Always
Steps to Reproduce:
1. Download & unpack 0.9.2
2. Click Next, Accept, Change, OK, Next, Install
3. Download begins

Actual Results:  > Gtk-CRITICAL **: file gtkprogress.c: line 518
(gtk_progress_set_percentage): assertion `percentage >= 0 && percentage <= 1.0'
failed.
> mozilla-installer: line 55: 6661 Segmentation fault ./mozilla-installer-bin
--sync $
> Gtk-CRITICAL **: file gtkwidget.c: line 1507 (gtk_widget_hide): assertion
`widget != NULL' failed.
> mozilla-installer: line 55: 6667 Segmentation fault ./mozilla-installer-bin
--sync $@

Expected Results:  Installed properly!  All previous versions (0.8, 0.8.1, 0.9,
0.9.1) installed fine.  This is the first one to break.

Comment 1

[Keyser Sose]

Marking NEW.

Comment 2

[Grace Bush]

this is duplicate of bug 83478

*** This bug has been marked as a duplicate of 83478 ***

Comment 3

[Daniel Veditz [:dveditz]]

This is not a duplicate of 83478, that one's a benign assertion, this one is a
segfault. (He gets the same assertion first, but that's just noise -- look at
what happens next.)

Comment 4

[Henry Throop]

This bug still exists.  Although I installed the 2001091311 0.9.4 distribution
successfully, as of today I can't install the most recent Linux nightly build. 
The message is:

mozilla-installer: line 55: 29035 Segmentation fault     
./mozilla-installer-bin --sync 

Comment 5

[Sean Su]

over to david.

Comment 6

[Syd Logan]

Attachment: patch to fix this

Ensure that the user only installs where permissions are appropriate. And
handle the error in the file picker where we should.

Comment 7

[Syd Logan]

reassigning to me

Comment 8

[Syd Logan]

Simple steps to reproduce:

su
mkdir /moztest
(ensure that only root and root's group have rw perms on /moztest, it should be
that way by default)
exit (log off as root)
run the stub or blob installer
select /moztest as the install location
continue on with the install, you should download all the files if using the
stub, but crash trying to process these files.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 63472 [details] [diff] [review]
patch to fix this

>+    if (0 == stat(selDir, &destStat))
>+        if (!S_ISDIR(destStat.st_mode) || VerifyDestination() != OK ) /* not a directory so don't tear down */
>+            return;

Please update this comment to indicate what the added VerifyDestination() check
is doing. Also the line's getting pretty long now, maybe move the comment
to its own line above or below the if (re-wording as necessary)

>+        if ( !(isRead && isWrite && isExecute) )

On IM you mentioned it'd have been better to use access(path, W_OK|R_OK|X_OK)
instead

Comment 10

[Syd Logan]

Comment on attachment 63472 [details] [diff] [review]
patch to fix this

I'm going to rewrite to use access(), which is simpler.

Comment 11

[Syd Logan]

Attachment: why implement access() when I can just call it?

Comment 12

[Syd Logan]

Comment on attachment 63563 [details] [diff] [review]
why implement access() when I can just call it?

ack, bug in this patch, one more try...

Comment 13

[Syd Logan]

Attachment: restored the stat() call, final patch

Needed the stat() call, that was what was checking for file existence. I took
it out in the previous patch assuming stat() was only being used to get the
permissions values, but on inspection, that was not the case.

Comment 14

[Syd Logan]

Attachment: added -w flag to get rid of whitespace changes

Comment 15

[Samir Gehani]

Comment on attachment 63576 [details] [diff] [review]
added -w flag to get rid of whitespace changes

>Index: nsSetupTypeDlg.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/xpinstall/wizard/unix/src2/nsSetupTypeDlg.cpp,v
>retrieving revision 1.18
>diff -w -u -2 -0 -r1.18 nsSetupTypeDlg.cpp
>--- nsSetupTypeDlg.cpp	14 Aug 2001 07:59:58 -0000	1.18
>+++ nsSetupTypeDlg.cpp	4 Jan 2002 22:11:17 -0000
>@@ -1,21 +1,21 @@
>-/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+

Looks like we are eliminating the mode line.  Let's restore it.  (Or remove it
from all the files under src2.)

>-    if (0 == stat(selDir, &destStat))
>-        if (!S_ISDIR(destStat.st_mode)) /* not a directory so don't tear down */
>-            return;
> 
>     strcpy(gCtx->opt->mDestination, selDir);
> 
>+    if (0 == stat(selDir, &destStat))
>+        if (!S_ISDIR(destStat.st_mode) || VerifyDestination() != OK ) /* not a directory so don't tear down */
>+            return;

Please comment about where VerifyDestination() is getting the destination to
verify would help code readability (or change VerifyDestination to take a path
and set mDestination only if it verifies correctly).

r=sgehani contingent upon above nits being addressed.

Comment 16

[Syd Logan]

I'll also add back in the mode line I removed by accident. I thought samir was
talking about the line that contained the stat() st_mode field, which was not
removed.Doh, sorry samir :-)

Comment 17

[Sean Su]

Comment on attachment 63576 [details] [diff] [review]
added -w flag to get rid of whitespace changes

I talked to syd.  There are things that could be done to make the code slightly
simpler, but for sake of not wanting to break anything else, r=ssu.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 63570 [details] [diff] [review]
restored the stat() call, final patch

>+    if (0 == stat(selDir, &destStat))
>+        if (!S_ISDIR(destStat.st_mode) || VerifyDestination() != OK ) /* not a directory so don't tear down */
>+            return;
>+

VerifyDestination() does the same stat(), so why not move the S_ISDIR
test inside VerifyDestination() and remove one of the stat() calls?
In addition, whatever it is that nsSetupTypeDlg::Next() is doing will
now also be doing the full correct check when it calls VerifyDestination
rather than lucking out because some other part of the code has checked
it's a directory.

And fix the long comment :-)  (What does it mean by "tear down" anyway?)

  if (VerifyDestination() != OK)
      return;

Comment 19

[Syd Logan]

I'd like to go for low risk. verify destination is called in one other place,
and not checking that S_ISDIR, so in the interest of minimizing risk, I'd rather
just fix the check inside the function to be correct, and add that extra call to
make sure we have permissions, and leave it at that.

Comment 20

[Daniel Veditz [:dveditz]]

My point was that the "one other place" (nsSetupTypeDlg::Next) is probably a bug
waiting to happen. Why *isn't* it checking S_ISDIR? Simplifying the code would
meet my definition of "low risk" for future problems.

But I could be argues your way, especially if this is a patch you'd like to
retrofit into a 6.2.1 installer re-spin.

sr=dveditz

Comment 21

[Syd Logan]

Fix checked in

Comment 22

[Doug Turner (:dougt)]

Comment on attachment 63576 [details] [diff] [review]
added -w flag to get rid of whitespace changes

why are you getting rid of the mode line.

 stat_err = stat(gCtx->opt->mDestination, &stbuf);
     if (stat_err == 0)
     {
-	 // check perms on dir: we need rwx for user
-	 if ( !(stbuf.st_mode & S_IREAD)  ||
-	      !(stbuf.st_mode & S_IWRITE) ||
-	      !(stbuf.st_mode & S_IEXEC)   ) 
+      if (access(gCtx->opt->mDestination, R_OK | W_OK | X_OK ) != 0)
	 {

Do you still need to do the stat?

Comment 23

[Grace Bush]

verified with build 2001010706