MAXPATHLEN too small for glibc's realpath()

REOPENED
Assigned to

Status

()

Toolkit
Startup and Profile System
REOPENED
10 years ago
7 years ago

People

(Reporter: wolfiR, Assigned: Sunil Mohan Adapa)

Tracking

({fixed1.9.0.12})

Trunk
mozilla1.9.2a1
x86
Linux
fixed1.9.0.12
Points:
---

Firefox Tracking Flags

(status1.9.1 ?)

Details

Attachments

(6 attachments, 9 obsolete attachments)

5.95 KB, patch
Details | Diff | Splinter Review
7.50 KB, patch
bsmedberg
: review-
Details | Diff | Splinter Review
2.47 KB, patch
Details | Diff | Splinter Review
2.99 KB, patch
Details | Diff | Splinter Review
5.19 KB, patch
Details | Diff | Splinter Review
5.21 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
(that also applies to 1.8)

With recent gcc and glibc versions (and certain compile options (_FORTIFY_SOURCE))
running Firefox aborts immediately because of:

#3  0x00007fff0c373397 in *__GI___fortify_fail (
    msg=0x7fff0c3abd2f "buffer overflow detected") at fortify_fail.c:32
No locals.
#4  0x00007fff0c3717a0 in *__GI___chk_fail () at chk_fail.c:29
No locals.
#5  0x00007fff0c371ebb in __realpath_chk (
    buf=0x71d1 <Address 0x71d1 out of bounds>, 
    resolved=0x71d1 <Address 0x71d1 out of bounds>, resolvedlen=6)
    at realpath_chk.c:30
No locals.
#6  0x0000000000448909 in XRE_GetBinaryPath (
    argv0=0x71d1 <Address 0x71d1 out of bounds>, aResult=0x7fff1a234a00)

That's in http://mxr.mozilla.org/seamonkey/source/toolkit/xre/nsAppRunner.cpp#1435

It seems MAXPATHLEN is 1024 here while glibc is using 4096 (AFAIK).
MAXPATHLEN in
http://mxr.mozilla.org/mozilla1.8/source/toolkit/xre/nsAppRunner.h#45
should use PATH_MAX as in
http://mxr.mozilla.org/mozilla1.8/source/xpcom/build/nsXPCOMPrivate.h#247
(Reporter)

Comment 1

10 years ago
I'm actually checking locally if that _really_ is the issue and will attach a patch then.
Yes, that's exactly the issue confirmed with jakub jelinek.
Created attachment 297358 [details] [diff] [review]
Patch from Martin Stransky

I thought stransky said he attached this to a bug before....  I can't find it though :(
(Reporter)

Comment 4

10 years ago
Comment on attachment 297358 [details] [diff] [review]
Patch from Martin Stransky

 
>diff -up mozilla/nsprpub/config/pathsub.h.old mozilla/nsprpub/config/pathsub.h
>--- mozilla/nsprpub/config/pathsub.h.old	2004-04-25 17:00:34.000000000 +0200
>+++ mozilla/nsprpub/config/pathsub.h	2007-09-25 18:57:51.000000000 +0200
>@@ -50,7 +50,7 @@
> #endif
> 
> #ifndef PATH_MAX
>-#define PATH_MAX 1024
>+#error  "PATH_MAX is not defined!"
> #endif

Is PATH_MAX available on all platforms we need to support (thinking about NSPR and NSS)?
So is it safe to remove that default?

Comment 5

9 years ago
I have run into the same issue with realpath().

I don't know whether PATH_MAX available on all platforms that Firefox needs to support, but the value 1024 is a very bad choice for default. Accordingly man for realpath(), the size of the buffer for it is defined as:

              #ifdef PATH_MAX
                path_max = PATH_MAX;
              #else
                path_max = pathconf (path, _PC_PATH_MAX);
                if (path_max <= 0)
                  path_max = 4096;
              #endif

So, 4096 would be much better value.

Besides, the definition of MAXPATHLEN is inconsistent. In nsXPCOMPrivate.h,
it uses PATH_MAX when it is available
http://mxr.mozilla.org/mozilla1.8/source/xpcom/build/nsXPCOMPrivate.h#248
but, in many other places, it does not...

IMHO, it would be better if MAXPATHLEN was defined as PATH_MAX when the latter is available... At least, this closes the problem for those platforms that have PATH_MAX defined.

Comment 6

9 years ago
Created attachment 309860 [details] [diff] [review]
define MAXPATHLEN as PATH_MAX when PATH_MAX is available

This patch solves the problem for those platforms that have PATH_MAX defined. It does not change anything for platforms that do not have PATH_MAX. The patch makes the definition of MAXPATHLEN the same as it is defined in nsXPCOMPrivate.h.

Comment 7

9 years ago
Comment on attachment 309860 [details] [diff] [review]
define MAXPATHLEN as PATH_MAX when PATH_MAX is available

Index: browser/components/migration/src/nsProfileMigrator.cpp
+#ifdef PATH_MAX
+#define MAXPATHLEN PATH_MAX
...
+#elif defined(PATH_MAX)
+#define MAXPATHLEN PATH_MAX

this is wrong :)

Comment 8

9 years ago
Created attachment 309958 [details] [diff] [review]
define MAXPATHLEN as PATH_MAX when PATH_MAX is available   

I am sorry for my mistake... I started adding definition of MAXPATHLEN as PATH_LEN after all others, but then saw that nsXPCOMPrivate.h does this in the different order. So, I decided to add "#define MAXPATHLEN PATH_MAX" at the beginning as in nsXPCOMPrivate.h but forgot remove the old lines. Here is a corrected version of that patch.
Attachment #309860 - Attachment is obsolete: true

Comment 9

9 years ago
Comment on attachment 309958 [details] [diff] [review]
define MAXPATHLEN as PATH_MAX when PATH_MAX is available   

well, someone should review this, and it won't be me.
Attachment #309958 - Flags: review?(shaver)
Duplicate of this bug: 418244

Comment 11

9 years ago
On mandriva, here is a patch that seems to work for firefox 3b1:
http://svn.mandriva.com/cgi-bin/viewvc.cgi/packages/cooker/mozilla-firefox/branches/firefox3/current/SOURCES/firefox-3.0rc1-glibc28-max_path-fix.patch?view=markup&pathrev=212641
Comment on attachment 309958 [details] [diff] [review]
define MAXPATHLEN as PATH_MAX when PATH_MAX is available   

I'd rather have Benjamin look at this, if possible -- I'm not au courant on the portability of things like limits.h
Attachment #309958 - Flags: review?(shaver) → review?(benjamin)
Attachment #309958 - Flags: review?(benjamin) → review+

Comment 13

9 years ago
Created attachment 325241 [details] [diff] [review]
Fix based on the patch used in Fedora

I found the patch to fix this in SRPM package from Fedora.

Here is the patch to fit to the latest trunk (be010ac940e0).
Attachment #325241 - Flags: review+

Updated

9 years ago
Attachment #325241 - Flags: review+ → review?

Updated

9 years ago
Attachment #325241 - Flags: review?

Updated

9 years ago
Attachment #325241 - Flags: review?(benjamin)
Comment on attachment 325241 [details] [diff] [review]
Fix based on the patch used in Fedora

How is this patch better than the first patch in this bug? In any case, the comment about "works for SCO" is no longer relevant with this patch.
Attachment #325241 - Flags: review?(benjamin) → review-

Comment 15

9 years ago
Oh, attachment 325241 [details] [diff] [review] fixes bug 421889.

I don't know which is the better fix, attachment 309958 [details] [diff] [review] or attachment 325241 [details] [diff] [review].
I only introduce how Fedora fixes this bug and hope to fix this.

Fedora 9 (glibc-2.8 and gcc-4.3.0) on x86_64 requires the attachment 325441 [details] [diff] [review] for getting well-worked firefox with the compiler option of -DFORTIFY_SOURCE=2.

Comment 16

9 years ago
bug 421889 comment #7 says bug 421889 is the dup. of this bug.

Updated

9 years ago
Component: XRE Startup → Startup and Profile System
QA Contact: xre.startup → startup
Created attachment 335369 [details] [diff] [review]
updated patch to work with mercurial queues
[Checkin: Comment 21]

This is exactly the same as attachment #309958 [details] [diff] [review] but updated to work with mercurial (queues).
The (old) patch has r+ and works fine, someone should get it pushed.
Attachment #309958 - Attachment is obsolete: true
Duplicate of this bug: 421889
I duped bug #421889 against this one.
Maybe someone can create a followup bug to use safer functions as suggested in
bug #421889 comment #7.
This ruined most of my day on a new ubuntu intrepid (beta) 64 bit build system. One of the fixes should be propagated. 

in my case MAXPATHLEN is already defined in sys/param.h - I cannot think of a reason unix platforms should not all use that.

iff -r a95a069f3372 xpcom/build/nsXPCOMPrivate.h
--- a/xpcom/build/nsXPCOMPrivate.h      Mon Sep 29 14:28:15 2008 -0400
+++ b/xpcom/build/nsXPCOMPrivate.h      Mon Sep 29 18:04:29 2008 -0400
@@ -260,7 +260,7 @@
   #error need_to_define_your_file_path_separator_and_illegal_characters
 #endif

-#ifdef AIX
+#if defined (XP_UNIX)
 #include <sys/param.h>
 #endif
http://hg.mozilla.org/mozilla-central/rev/adf33dbb59d0

Thanks for the patch!

(In reply to comment #20)
> in my case MAXPATHLEN is already defined in sys/param.h - I cannot think of a
> reason unix platforms should not all use that.

Want to file another bug on changing this, please? Thanks!
Assignee: nobody → dpotapov
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
(Assignee)

Comment 22

9 years ago
Created attachment 355004 [details] [diff] [review]
Patch covering more cases on the lines of the one checked in

I built Spicebird for Fedora 10 and encountered this crash even with the fix checked in above. A quick grep indicated that some cases may not have been covered by the checked-in patch (although some of the patches in this bug seem to touch upon them). So, I made a patch a long the lines of the checked in patch that covers others cases also (esp. the one in sqlite module).

The stack trace for the crash I encountered in f10 is as follows: 

#0  0x0000003281632ed5 in raise () from /lib64/libc.so.6
#1  0x0000003281634a43 in abort () from /lib64/libc.so.6
#2  0x0000003281672408 in __libc_message () from /lib64/libc.so.6
#3  0x00000032816ff497 in __fortify_fail () from /lib64/libc.so.6
#4  0x00000032816fd340 in __chk_fail () from /lib64/libc.so.6
#5  0x00000032816fd9fb in __realpath_chk () from /lib64/libc.so.6
#6  0x000000000043d8a6 in sqlite3_bind_null ()
#7  0x000000000043cc5f in sqlite3_bind_null ()
#8  0x000000328161e546 in __libc_start_main () from /lib64/libc.so.6
#9  0x000000000043cac9 in sqlite3_bind_null ()
#10 0x00007fffffffe0a8 in ?? ()
#11 0x000000000000001c in ?? ()
#12 0x0000000000000001 in ?? ()
#13 0x00007fffffffe3a3 in ?? ()
#14 0x0000000000000000 in ?? ()
Attachment #355004 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #355004 - Flags: review? → review?(benjamin)
(Assignee)

Comment 23

9 years ago
Created attachment 355007 [details] [diff] [review]
Patch for mail/migration and mailnew/import on the lines of browser/migration

Similar patch for mail/components/migration and mailnews/import.
Attachment #355007 - Flags: review?
(Assignee)

Comment 24

9 years ago
I just recollect that the crash occurred without the fix that has been checked in. So, I can't say for sure that the changes in the above 2 patches are required. However, the stack trace seems to suggests that.
Attachment #355004 - Flags: review?(benjamin) → review-
Comment on attachment 355004 [details] [diff] [review]
Patch covering more cases on the lines of the one checked in

>Index: db/sqlite3/src/sqlite3.c

The sqlite3 code is imported from SQLite without modification. Please file and fix this upstream, and we'll pick it up at the next release update.

>Index: modules/lcms/include/lcms.h

LCMS is also external code.

I'm technically not a JS peer, but I think I can review this patch for jsfile.cpp given the circumstances and content.

Comment 26

9 years ago
(In reply to comment #25)
> >Index: modules/lcms/include/lcms.h
> 
> LCMS is also external code.
> 
> I'm technically not a JS peer, but I think I can review this patch for
> jsfile.cpp given the circumstances and content.

Now LCMS used by mozilla is modified by Bobby in many parts and we cannot use external one for building mozilla.  See bug 469558.

If this fix for lcms.h is required, we need to make a patch which include only lcms.h part and request review to Bobby.
(Assignee)

Comment 27

9 years ago
Created attachment 355537 [details] [diff] [review]
jsfile.cpp part of the earlier patch

Thank you for your comments.

> The sqlite3 code is imported from SQLite without modification. Please file and
> fix this upstream, and we'll pick it up at the next release update.

I commented on an existing bug here:
http://www.sqlite.org/cvstrac/tktview?tn=3373

> I'm technically not a JS peer, but I think I can review this patch for
> jsfile.cpp given the circumstances and content.

I am splitting the patch into individual modules and uploading. I am asking for your review on the jsfile.cpp. Sorry about grouped patch earlier.
Attachment #355004 - Attachment is obsolete: true
Attachment #355537 - Flags: review?(benjamin)
(Assignee)

Updated

9 years ago
Attachment #355007 - Flags: review? → review?(dmose)
(Assignee)

Comment 28

9 years ago
Created attachment 355538 [details] [diff] [review]
lcms part of the earlier patch

I can't confirm that the patch is absolutely required because I don't have crashes and stack traces for all the changes.

I could not find Bobby on module owners page to ask for review.
Attachment #355538 - Flags: review?(bholley)
(Assignee)

Comment 29

9 years ago
Created attachment 355539 [details] [diff] [review]
modules/libreg part of the earlier patch
Attachment #355539 - Flags: review?
(Assignee)

Comment 30

9 years ago
Created attachment 355541 [details] [diff] [review]
widget part of the earlier patch
Attachment #355541 - Flags: review?
(Assignee)

Comment 31

9 years ago
Created attachment 355542 [details] [diff] [review]
xpcom part of the earlier patch
Attachment #355542 - Flags: review?(benjamin)
Attachment #355537 - Flags: review?(benjamin) → review+
Assignee: dpotapov → sunil
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #355542 - Flags: review?(benjamin) → review+
(Assignee)

Updated

9 years ago
Attachment #355541 - Flags: review? → review?(roc)
(Assignee)

Updated

9 years ago
Attachment #355539 - Flags: review? → review?(dveditz)
Attachment #355541 - Flags: review?(roc) → review+
Comment on attachment 355538 [details] [diff] [review]
lcms part of the earlier patch

looks good. r=bholley on the lcms part.
Attachment #355538 - Flags: review?(bholley) → review+
Comment on attachment 355007 [details] [diff] [review]
Patch for mail/migration and mailnew/import on the lines of browser/migration

r+sr=dmose; thanks for the patch
Attachment #355007 - Flags: superreview+
Attachment #355007 - Flags: review?(dmose)
Attachment #355007 - Flags: review+
(Assignee)

Comment 34

9 years ago
I could not find anyone to review the modules/libreg part of the patch, can someone suggest a reviewer? Also, I guess superreview is need for last four patches. Benjamin, can I trouble you for that, since you looked at the some patches already?
Attachment #355539 - Flags: review?(dveditz) → review+
general sr=me
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Please, flag as obsolete / (visibly) mark as checked in / etc, then state which patch(s) to c-n.
(Assignee)

Comment 37

8 years ago
Checked in:
==========

attachment 335369 [details] [diff] [review]

Check in needed:
===============

attachment 355007 [details] [diff] [review]
attachment 355537 [details] [diff] [review]
attachment 355538 [details] [diff] [review]
attachment 355539 [details] [diff] [review]
attachment 355541 [details] [diff] [review]
attachment 355542 [details] [diff] [review]

Summary of the bug so far:
=========================

One chunk in sqlite3 needs fixing in upstream, later it could be imported into Mozilla code. http://www.sqlite.org/cvstrac/tktview?tn=3373

Changes that were suggested in the initial patch (attachment 297358 [details] [diff] [review]) and are not  checked-in are in following files:
config/pathsub.h
nsprpub/config/pathsub.h
security/coreconf/nsinstall/pathsub.h
xpcom/obsolete/nsFileSpecUnix.cpp
dbm/include/mcom_db.h
xpcom/typelib/xpidl/xpidl_java.c
modules/libjar/nsZipArchive.cpp
without those patches, xulrunner/firefox-3.0.6 and firefox-2.20 fails to launch due to this on Ubuntu Intreip, which is really bad once many products rely on these versions of firefox.

flagging for request 3.0.7 blocker at least.
Flags: blocking1.9.0.7?
Attachment #335369 - Attachment description: updated patch to work with mercurial queues → updated patch to work with mercurial queues [Checkin: Comment 21]
(In reply to comment #37)

Could you attach 1 m-c and 1 c-c Hg diffs, with detailed comments ?

> attachment 355007 [details] [diff] [review]

Why is this one already marked 'checked-in' ?
(Assignee)

Comment 40

8 years ago
Comment on attachment 355007 [details] [diff] [review]
Patch for mail/migration and mailnew/import on the lines of browser/migration

> > attachment 355007 [details] [diff] [review] [details]

> Why is this one already marked 'checked-in' ?

I didn't know checked-in keyword is used to mark already checked-in patches. I used it to mean "... similar to a patch that is already checked-in". I removed it now.
Attachment #355007 - Attachment description: Patch for mail/migration and mailnew/import on the lines of browser/migration checked-in → Patch for mail/migration and mailnew/import on the lines of browser/migration
(Assignee)

Comment 41

8 years ago
Created attachment 363288 [details] [diff] [review]
Comm-central patch of attachment 355007 [details] [diff] [review] for fixes in mail/migration and mailnews/import
[Checkin: Comment 47]

This patch is hg diff created from attachment 355007 [details] [diff] [review] (already reviewed, checkin ready). It contains fixes for the following modules:

mail/migration
mailnews/import
Attachment #355007 - Attachment is obsolete: true
(Assignee)

Comment 42

8 years ago
Created attachment 363289 [details] [diff] [review]
Moz-central patch combining attachments (355537  355538 355539 355541 355542)
[Checkin: Comment 46]

This patch is a moz-central hg diff of attachments 355537, 355538, 355539, 355541 and 355542. All these patches have been reviewed and are ready for check in. They fix the problems in following modules:

js/
modules/lcms/
modules/libreg/
widget/src/
xpcom/io/
Attachment #355537 - Attachment is obsolete: true
Attachment #355538 - Attachment is obsolete: true
Attachment #355539 - Attachment is obsolete: true
Attachment #355541 - Attachment is obsolete: true
Attachment #355542 - Attachment is obsolete: true
Not blocking a 1.9.0.x release. Request branch approval on a patch after it has landed and been tested on the trunk (we don't have enough nightly testers on the 1.9.0 branch to safely take untested non-critical patches).
Flags: blocking1.9.0.7?
Do the two patches need to land simultaneously ?
(Assignee)

Comment 45

8 years ago
(In reply to comment #44)
> Do the two patches need to land simultaneously ?

No, they are independent of one another.
Comment on attachment 363289 [details] [diff] [review]
Moz-central patch combining attachments (355537  355538 355539 355541 355542)
[Checkin: Comment 46]


http://hg.mozilla.org/mozilla-central/rev/edb1bcc890c0
Attachment #363289 - Attachment description: Moz-central patch combining attachments (355537 355538 355539 355541 355542) → Moz-central patch combining attachments (355537 355538 355539 355541 355542) [Checkin: Comment 46]
Comment on attachment 363288 [details] [diff] [review]
Comm-central patch of attachment 355007 [details] [diff] [review] for fixes in mail/migration and mailnews/import
[Checkin: Comment 47]


http://hg.mozilla.org/comm-central/rev/d7831ab9ae38
Attachment #363288 - Attachment description: Comm-central patch of attachment 355007 for fixes in mail/migration and mailnews/import → Comm-central patch of attachment 355007 for fixes in mail/migration and mailnews/import [Checkin: Comment 47]
(In reply to comment #37)
> One chunk in sqlite3 needs fixing in upstream, later it could be imported into
> Mozilla code. http://www.sqlite.org/cvstrac/tktview?tn=3373

You may want to file a dependent bug.

> Changes that were suggested in the initial patch (attachment 297358 [details] [diff] [review]) and are
> not  checked-in are in following files:

Leaving bug open, as I don't know what you plan to do about these.
Keywords: checkin-needed
Target Milestone: mozilla1.9.1b2 → mozilla1.9.2a1
Created attachment 374325 [details] [diff] [review]
190x version of "Moz-central patch combining attachments"

Here's a backported patch for 1.9.0.x, based on the mozilla-central patch.

(The posted mozilla-central patch *almost* applies cleanly to 190x -- I just had to retarget the "jsfile.cpp" chunk to "jsfile.c" instead, because that file was renamed on mozilla-central in bug 387935 / changeset bd9c9cbf9ec8.)
Comment on attachment 374325 [details] [diff] [review]
190x version of "Moz-central patch combining attachments"

The backported 1.9.0.x patch doesn't completely fix the bug there -- I still get an immediate startup crash in an optimized CVS trunk build, with attachment 374325 [details] [diff] [review] applied.

*** buffer overflow detected ***: ./dist/bin/firefox-bin terminated
======= Backtrace: =========
/lib/tls/i686/cmov/libc.so.6(__fortify_fail+0x48)[0xb7357da8]
[snip]

So it looks like more needs to be done to fix this on 1.9.0.x (if we want to fix this on 1.9.0.x)

(For anyone else hitting this bug, you can work around it by using gcc / g++ version 4.2 instead of 4.3)
Attachment #374325 - Attachment description: 190x patch → Backported 190x patch v1 [doesn't completely fix bug]
http://hg.mozilla.org/users/dbaron_mozilla.com/patches-1.9/raw-file/4d2adcbc307d/maxpathlen-fix has worked for me for a while as a patch backported to 1.9.0.
(In reply to comment #51)
> http://hg.mozilla.org/users/dbaron_mozilla.com/patches-1.9/raw-file/4d2adcbc307d/maxpathlen-fix
> has worked for me for a while as a patch backported to 1.9.0.

Ah, that's actually exactly attachment 335369 [details] [diff] [review] (but with more contextual lines).  I missed that patch in the attachment list, amid all the obsoleted patches surrounding/following it. :)

So it looks like *that* patch landed on mozilla-central a while ago (in comment 21), and the later patch that I backported is extra stuff that was discovered to also need fixing so that other mozilla-based apps will work (per comment 22).

So if I understand correctly, 1.9.0.x needs both attachment 335369 [details] [diff] [review] and attachment 374325 [details] [diff] [review]. (though Firefox itself only needs the first of those two in order to work)
Attachment #374325 - Attachment description: Backported 190x patch v1 [doesn't completely fix bug] → 190x patch combining attachments (backported)
Attachment #374325 - Flags: approval1.9.0.12?
Attachment #335369 - Flags: approval1.9.0.12?
The 1.9.0 release drivers are confused. Is this or is this not fixed on trunk? Checkins appear to have happened but the bug is in a reopened state.

Has this been fixed in 1.9.1 ? If not why not? If it's not important enough to get into the active branch (I see no approval or blocking requests) why is it important to land on 1.9.0?
Comment on attachment 374325 [details] [diff] [review]
190x version of "Moz-central patch combining attachments"

(In reply to comment #53)
> Checkins appear to have happened but the bug is in a reopened state.
This was originally closed in comment 21, when the initial patch landed.  It was later reopened, as more similar patches were posted to fix this in more places.

The last "new" patch was landed in comment 48, but the bug was left open because additional fixes were possibly still coming.

> Has this been fixed in 1.9.1 ? If not why not? If it's not important enough to
> get into the active branch (I see no approval or blocking requests) why is it
> important to land on 1.9.0?

Actually, it looks like it hasn't!  The first patch landed before 1.9.1 split off (in comment 21), so that one made it into 1.9.1, but the later patches did not.

However, I think this is "fixed" on 1.9.1 in the sense that I can start a 1.9.1 build without immediately crashing.  So perhaps the first patch is all that was needed there? (and all that would need backporting to 1.9.0.x)

Anyway -- canceling 190x approval request for now, since the patch I requested approval on contains stuff that isn't in 1.9.1. (the second round of patches on this bug)
Attachment #374325 - Flags: approval1.9.0.12?
(In reply to comment #54)
> However, I think this is "fixed" on 1.9.1 in the sense that I can start a 1.9.1
> build without immediately crashing.  So perhaps the first patch is all that was
> needed there? (and all that would need backporting to 1.9.0.x)

Ah -- yes, the above is correct, based on Comment 52.

So, to sum up:
 - The patch in attachment 335369 [details] [diff] [review] fixes this bug *in Firefox*
 - That patch is already in both mozilla-central and 1.9.1.
 - That patch needs approval1.9.0.x, to fix this bug in Firefox 3.0.x
 - (The later patches are for correctness, to fix other places where this bug could hypothetically pop up in mozilla-based software.  However, those later patches don't affect Firefox's behavior, and they aren't included in 1.9.1.)

Hence, I'm leaving the approval request open on attachment 335369 [details] [diff] [review].
Comment on attachment 335369 [details] [diff] [review]
updated patch to work with mercurial queues
[Checkin: Comment 21]

Approved for 1.9.0.12, a=dveditz for release-drivers
Attachment #335369 - Flags: approval1.9.0.12? → approval1.9.0.12+
Comment on attachment 335369 [details] [diff] [review]
updated patch to work with mercurial queues
[Checkin: Comment 21]

Landed on CVS trunk for 1.9.0.12:
mozilla/browser/components/migration/src/nsDogbertProfileMigrator.cpp 	1.29
mozilla/browser/components/migration/src/nsProfileMigrator.cpp 	1.27
mozilla/toolkit/mozapps/update/src/updater/updater.cpp 	1.40
mozilla/toolkit/xre/nsAppRunner.h 	1.27
mozilla/xpcom/build/nsXPCOMPrivate.h 	1.47

Updated

8 years ago
Keywords: fixed1.9.0.12
Depends on: 501800
No longer depends on: 501800
Depends on: 502723
(In reply to comment #55)
>  - (The later patches are for correctness, to fix other places where this bug
> could hypothetically pop up in mozilla-based software.  However, those later
> patches don't affect Firefox's behavior, and they aren't included in 1.9.1.)

Daniel: Do we want the later patches (for correctness) on 1.9.1?
Flags: wanted1.9.1.x?
Attachment #374325 - Attachment description: 190x patch combining attachments (backported) → 190x version of "Moz-central patch combining attachments"
If the later patches are landed on 1.9.1 please also land the fix in bug 502723 that this bug introduced. Thanks

http://mxr.mozilla.org/mozilla1.9.1/source/toolkit/mozapps/update/src/updater/updater.cpp#123
(In reply to comment #58)
> Daniel: Do we want the later patches (for correctness) on 1.9.1?

First of all, to be clear -- there's actually only one additional patch that'd be under consideration here, for the branches: attachment 363289 [details] [diff] [review] (and its backported-to-1.9.0 version, attachment 374325 [details] [diff] [review])  All other patches here are either obsolete (from merging into a combined patch), already landed on all branches, or are for comm-central.

In response to Sam's question, I'm not really sure whether attachment 363289 [details] [diff] [review] is branch-appropriate.  I haven't run into any crashes caused by its absence, and I'm not entirely sure in what situations it's needed. (Comment 22 suggests that SpiceBird needs it -- maybe Thunderbird as well?)

The patch author & reviewers would probably be better judges / vouchers than I, regarding the appropriateness on the branches.

(In reply to comment #59)
> If the later patches are landed on 1.9.1 please also land the fix in bug 502723
> that this bug introduced

That (minor) issue was actually from attachment 335369 [details] [diff] [review], the patch that's already included in both 1.9.1 and 1.9.0.  Hence, I'm requesting branch approval on bug 502723's patch.  (any further discussion on that should go on bug 502723)
status1.9.1: --- → ?
Flags: wanted1.9.1.x?

Updated

7 years ago
You need to log in before you can comment on or make changes to this bug.