Last Comment Bug 412610 - MAXPATHLEN too small for glibc's realpath()
: MAXPATHLEN too small for glibc's realpath()
Status: REOPENED
: fixed1.9.0.12
Product: Toolkit
Classification: Components
Component: Startup and Profile System (show other bugs)
: Trunk
: x86 Linux
: -- normal with 1 vote (vote)
: mozilla1.9.2a1
Assigned To: Sunil Mohan Adapa
:
Mentors:
: 418244 421889 (view as bug list)
Depends on: 502723
Blocks:
  Show dependency treegraph
 
Reported: 2008-01-16 07:55 PST by Wolfgang Rosenauer [:wolfiR]
Modified: 2010-09-17 13:38 PDT (History)
27 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
?


Attachments
Patch from Martin Stransky (5.95 KB, patch)
2008-01-16 08:18 PST, Christopher Aillon (sabbatical, not receiving bugmail)
no flags Details | Diff | Review
define MAXPATHLEN as PATH_MAX when PATH_MAX is available (4.57 KB, patch)
2008-03-16 20:40 PDT, Dmitry Potapov
no flags Details | Diff | Review
define MAXPATHLEN as PATH_MAX when PATH_MAX is available (4.41 KB, patch)
2008-03-17 06:35 PDT, Dmitry Potapov
benjamin: review+
Details | Diff | Review
Fix based on the patch used in Fedora (7.50 KB, patch)
2008-06-16 02:29 PDT, Takanori MATSUURA
benjamin: review-
Details | Diff | Review
updated patch to work with mercurial queues [Checkin: Comment 21] (2.47 KB, patch)
2008-08-25 09:07 PDT, Arpad Borsos [:Swatinem]
dveditz: approval1.9.0.12+
Details | Diff | Review
Patch covering more cases on the lines of the one checked in (7.47 KB, patch)
2009-01-01 00:09 PST, Sunil Mohan Adapa
benjamin: review-
Details | Diff | Review
Patch for mail/migration and mailnew/import on the lines of browser/migration (3.60 KB, patch)
2009-01-01 00:21 PST, Sunil Mohan Adapa
dmose: review+
dmose: superreview+
Details | Diff | Review
jsfile.cpp part of the earlier patch (1.54 KB, patch)
2009-01-06 00:22 PST, Sunil Mohan Adapa
benjamin: review+
Details | Diff | Review
lcms part of the earlier patch (1.24 KB, patch)
2009-01-06 00:32 PST, Sunil Mohan Adapa
bobbyholley: review+
Details | Diff | Review
modules/libreg part of the earlier patch (1.04 KB, patch)
2009-01-06 00:33 PST, Sunil Mohan Adapa
benjamin: review+
Details | Diff | Review
widget part of the earlier patch (1.34 KB, patch)
2009-01-06 00:35 PST, Sunil Mohan Adapa
roc: review+
Details | Diff | Review
xpcom part of the earlier patch (1.05 KB, patch)
2009-01-06 00:36 PST, Sunil Mohan Adapa
benjamin: review+
Details | Diff | Review
Comm-central patch of attachment 355007 for fixes in mail/migration and mailnews/import [Checkin: Comment 47] (2.99 KB, patch)
2009-02-20 04:02 PST, Sunil Mohan Adapa
no flags Details | Diff | Review
Moz-central patch combining attachments (355537 355538 355539 355541 355542) [Checkin: Comment 46] (5.19 KB, patch)
2009-02-20 04:08 PST, Sunil Mohan Adapa
no flags Details | Diff | Review
190x version of "Moz-central patch combining attachments" (5.21 KB, patch)
2009-04-23 13:05 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Review

Description Wolfgang Rosenauer [:wolfiR] 2008-01-16 07:55:37 PST
(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
Comment 1 Wolfgang Rosenauer [:wolfiR] 2008-01-16 08:05:58 PST
I'm actually checking locally if that _really_ is the issue and will attach a patch then.
Comment 2 Christopher Aillon (sabbatical, not receiving bugmail) 2008-01-16 08:14:30 PST
Yes, that's exactly the issue confirmed with jakub jelinek.
Comment 3 Christopher Aillon (sabbatical, not receiving bugmail) 2008-01-16 08:18:07 PST
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 :(
Comment 4 Wolfgang Rosenauer [:wolfiR] 2008-01-16 10:01:17 PST
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 Dmitry Potapov 2008-03-16 20:31:14 PDT
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 Dmitry Potapov 2008-03-16 20:40:43 PDT
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 timeless 2008-03-17 01:50:02 PDT
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 Dmitry Potapov 2008-03-17 06:35:30 PDT
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.
Comment 9 timeless 2008-03-31 00:02:32 PDT
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.
Comment 10 Christopher Aillon (sabbatical, not receiving bugmail) 2008-05-22 12:38:24 PDT
*** Bug 418244 has been marked as a duplicate of this bug. ***
Comment 12 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-06-05 06:51:38 PDT
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
Comment 13 Takanori MATSUURA 2008-06-16 02:29:22 PDT
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).
Comment 14 Benjamin Smedberg [:bsmedberg] 2008-06-18 06:43:06 PDT
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.
Comment 15 Takanori MATSUURA 2008-06-18 07:08:12 PDT
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 Takanori MATSUURA 2008-06-25 01:47:28 PDT
bug 421889 comment #7 says bug 421889 is the dup. of this bug.
Comment 17 Arpad Borsos [:Swatinem] 2008-08-25 09:07:08 PDT
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.
Comment 18 Arpad Borsos [:Swatinem] 2008-08-25 09:09:48 PDT
*** Bug 421889 has been marked as a duplicate of this bug. ***
Comment 19 Arpad Borsos [:Swatinem] 2008-08-25 09:12:44 PDT
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.
Comment 20 Patrick McManus [:mcmanus] 2008-09-29 15:06:26 PDT
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
Comment 21 Reed Loden [:reed] (use needinfo?) 2008-10-20 20:57:30 PDT
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!
Comment 22 Sunil Mohan Adapa 2009-01-01 00:09:24 PST
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 ?? ()
Comment 23 Sunil Mohan Adapa 2009-01-01 00:21:26 PST
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.
Comment 24 Sunil Mohan Adapa 2009-01-01 00:33:06 PST
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.
Comment 25 Benjamin Smedberg [:bsmedberg] 2009-01-05 10:48:57 PST
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 Takanori MATSUURA 2009-01-05 16:47:08 PST
(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.
Comment 27 Sunil Mohan Adapa 2009-01-06 00:22:27 PST
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.
Comment 28 Sunil Mohan Adapa 2009-01-06 00:32:34 PST
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.
Comment 29 Sunil Mohan Adapa 2009-01-06 00:33:59 PST
Created attachment 355539 [details] [diff] [review]
modules/libreg part of the earlier patch
Comment 30 Sunil Mohan Adapa 2009-01-06 00:35:11 PST
Created attachment 355541 [details] [diff] [review]
widget part of the earlier patch
Comment 31 Sunil Mohan Adapa 2009-01-06 00:36:07 PST
Created attachment 355542 [details] [diff] [review]
xpcom part of the earlier patch
Comment 32 Bobby Holley (PTO through June 13) 2009-01-06 10:41:48 PST
Comment on attachment 355538 [details] [diff] [review]
lcms part of the earlier patch

looks good. r=bholley on the lcms part.
Comment 33 Dan Mosedale (:dmose) 2009-01-06 18:15:47 PST
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
Comment 34 Sunil Mohan Adapa 2009-01-21 02:05:16 PST
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?
Comment 35 Benjamin Smedberg [:bsmedberg] 2009-01-21 07:32:08 PST
general sr=me
Comment 36 Serge Gautherie (:sgautherie) 2009-02-15 17:44:19 PST
Please, flag as obsolete / (visibly) mark as checked in / etc, then state which patch(s) to c-n.
Comment 37 Sunil Mohan Adapa 2009-02-17 02:25:42 PST
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
Comment 38 Antonio Gomes (tonikitoo) 2009-02-18 19:18:17 PST
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.
Comment 39 Serge Gautherie (:sgautherie) 2009-02-19 08:39:07 PST
(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' ?
Comment 40 Sunil Mohan Adapa 2009-02-20 03:58:42 PST
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.
Comment 41 Sunil Mohan Adapa 2009-02-20 04:02:36 PST
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
Comment 42 Sunil Mohan Adapa 2009-02-20 04:08:38 PST
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/
Comment 43 Daniel Veditz [:dveditz] 2009-02-20 11:28:52 PST
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).
Comment 44 Serge Gautherie (:sgautherie) 2009-02-20 18:23:16 PST
Do the two patches need to land simultaneously ?
Comment 45 Sunil Mohan Adapa 2009-02-23 02:06:36 PST
(In reply to comment #44)
> Do the two patches need to land simultaneously ?

No, they are independent of one another.
Comment 46 Serge Gautherie (:sgautherie) 2009-02-23 11:46:21 PST
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
Comment 47 Serge Gautherie (:sgautherie) 2009-02-23 11:51:47 PST
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
Comment 48 Serge Gautherie (:sgautherie) 2009-02-23 11:55:42 PST
(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.
Comment 49 Daniel Holbert [:dholbert] 2009-04-23 13:05:32 PDT
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 50 Daniel Holbert [:dholbert] 2009-04-24 16:29:29 PDT
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)
Comment 51 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-04-24 17:05:46 PDT
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.
Comment 52 Daniel Holbert [:dholbert] 2009-04-24 17:40:30 PDT
(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)
Comment 53 Daniel Veditz [:dveditz] 2009-05-28 11:39:48 PDT
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 54 Daniel Holbert [:dholbert] 2009-05-28 11:53:29 PDT
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)
Comment 55 Daniel Holbert [:dholbert] 2009-05-28 12:14:52 PDT
(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 56 Daniel Veditz [:dveditz] 2009-06-03 16:17:51 PDT
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
Comment 57 Mats Palmgren (:mats) 2009-06-05 08:26:21 PDT
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
Comment 58 Samuel Sidler (old account; do not CC) 2009-07-06 20:34:56 PDT
(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?
Comment 59 Robert Strong [:rstrong] (use needinfo to contact me) 2009-07-06 20:50:52 PDT
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
Comment 60 Daniel Holbert [:dholbert] 2009-07-06 21:02:36 PDT
(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)

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