fails to run when built with FORTIFY_SOURCE

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
Patch from Martin Stransky

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

Comment on attachment 297358
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?

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.

Created attachment 309860
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 on attachment 309860
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 :)

Created attachment 309958
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 on attachment 309958
define MAXPATHLEN as PATH_MAX when PATH_MAX is available

well, someone should review this, and it won't be me.

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

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
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

Created attachment 325241
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 on attachment 325241
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.

Oh, attachment 325241 fixes bug 421889.

I don't know which is the better fix, attachment 309958 or attachment 325241.
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 for getting well-worked firefox with the compiler option of -DFORTIFY_SOURCE=2.

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

Created attachment 335369
updated patch to work with mercurial queues
[Checkin: Comment 21]

This is exactly the same as attachment #309958 but updated to work with mercurial (queues).
The (old) patch has r+ and works fine, someone should get it pushed.

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

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!

Created attachment 355004
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 ?? ()

Created attachment 355007
Patch for mail/migration and mailnew/import on the lines of browser/migration

Similar patch for mail/components/migration and mailnews/import.

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 on attachment 355004
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.

(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.

Created attachment 355537
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.

Created attachment 355538
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.

Created attachment 355539
modules/libreg part of the earlier patch

Created attachment 355541
widget part of the earlier patch

Created attachment 355542
xpcom part of the earlier patch

Comment on attachment 355538
lcms part of the earlier patch

looks good. r=bholley on the lcms part.

Comment on attachment 355007
Patch for mail/migration and mailnew/import on the lines of browser/migration

r+sr=dmose; thanks for the patch

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?

general sr=me

Please, flag as obsolete / (visibly) mark as checked in / etc, then state which patch(s) to c-n.

Checked in:
==========

attachment 335369

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

attachment 355007
attachment 355537
attachment 355538
attachment 355539
attachment 355541
attachment 355542

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) 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.

(In reply to comment #37)

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

> attachment 355007 [details]

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

Comment on attachment 355007
Patch for mail/migration and mailnew/import on the lines of browser/migration

> > attachment 355007 [details] [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.

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

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

mail/migration
mailnews/import

Created attachment 363289
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/

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).

Do the two patches need to land simultaneously ?

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

No, they are independent of one another.

Comment on attachment 363289
Moz-central patch combining attachments (355537 355538 355539 355541 355542)
[Checkin: Comment 46]

http://hg.mozilla.org/mozilla-central/rev/edb1bcc890c0

Comment on attachment 363288
Comm-central patch of attachment 355007 for fixes in mail/migration and mailnews/import
[Checkin: Comment 47]

http://hg.mozilla.org/comm-central/rev/d7831ab9ae38

(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]) and are
> not checked-in are in following files:

Leaving bug open, as I don't know what you plan to do about these.

Created attachment 374325
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
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 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)

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 (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 and attachment 374325. (though Firefox itself only needs the first of those two in order to work)

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
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)

(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 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.

Comment on attachment 335369
updated patch to work with mercurial queues
[Checkin: Comment 21]

Approved for 1.9.0.12, a=dveditz for release-drivers

Comment on attachment 335369
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

(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?

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 (and its backported-to-1.9.0 version, attachment 374325) 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 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, 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)

The bug assignee didn't login in Bugzilla in the last 7 months.
:mossop, could you have a look please?
For more information, please visit [auto_nag documentation](https://wiki.mozilla.org/Release_Management/autonag#assignee_no_login.py).

Sounds like maybe this is actually resolved?

I think so, yeah.