Use of major & minor macros without including sys/sysmacros.h

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: anthonyryan1, Assigned: mgorny)

Tracking

50 Branch
mozilla54
Points:
---

Firefox Tracking Flags

(firefox52- fixed, firefox-esr52 fixed, firefox53- fixed, firefox54 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
User Agent: 
Build ID: 20161228001411

Steps to reproduce:

An upcoming version of glibc intends to stop including sys/sysmacros.h automatically with sys/type.h and require that it be included explicitly for the use of the major, minor & makedev macros.

Compiling firefox against a version of glibc with this implicit inclusion removed causes a compiler error.


Actual results:

Compiler error: 
/var/tmp/portage/www-client/firefox-50.1.0-r1/work/firefox-50.1.0/xpcom/io/nsLocalFileUnix.cpp:1380:46: error: 'major' was not declared in this scope
   if (!GetDeviceName(major(mCachedStat.st_dev),
                                              ^
/var/tmp/portage/www-client/firefox-50.1.0-r1/work/firefox-50.1.0/xpcom/io/nsLocalFileUnix.cpp:1381:46: error: 'minor' was not declared in this scope
                      minor(mCachedStat.st_dev),
                                              ^



Expected results:

The file should

#include <sys/sysmacros.h>

This will also improve compatibility with alternative libc implementations.

Updated

2 years ago
Component: Untriaged → XPCOM
Product: Firefox → Core
(Assignee)

Comment 1

2 years ago
Created attachment 8832788 [details] [diff] [review]
0001-Include-sys-sysmacros.h-for-major-minor-when-availab.patch

Here's a patch adding the #include conditionally to a configure check, and adding the file to system-includes. I have confirmed that it fixes the build on Linux with patched glibc.

Updated

2 years ago
Attachment #8832788 - Flags: review?(mh+mozilla)
Comment on attachment 8832788 [details] [diff] [review]
0001-Include-sys-sysmacros.h-for-major-minor-when-availab.patch

Review of attachment 8832788 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/io/nsLocalFileUnix.cpp
@@ +30,5 @@
>  #define BLOCK_SIZE 1024 /* kernel block size */
>  #endif
>  #endif
> +#if defined(HAVE_SYS_SYSMACROS_H)
> +#include <sys/sysmacros.h>

The macros major and minor are only used in portions of the code that require linux/quota.h. Which pretty much assumes a Linux target system.

Looking at the most popular non-glibc Linux libcs (bionic, musl, uclibc), they all provide sys/sysmacros.h. So let's just not check it exists. We can pretty much assumes it does.

Just place the include in the same #if as the sys/quota.h include.
Attachment #8832788 - Flags: review?(mh+mozilla)
(Assignee)

Comment 3

2 years ago
Created attachment 8837466 [details] [diff] [review]
0001-Include-sys-sysmacros.h-for-major-minor-on-Linux.patch
Attachment #8832788 - Attachment is obsolete: true

Updated

2 years ago
Attachment #8837466 - Flags: review?(mh+mozilla)
Attachment #8837466 - Flags: review?(mh+mozilla) → review+

Updated

2 years ago
Keywords: checkin-needed

Comment 4

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9a1da5e4f89
Include sys/sysmacros.h for major(), minor() on Linux. r=glandium
Keywords: checkin-needed

Comment 5

2 years ago
[Tracking Requested - why for this release]:

As many distros are already moving to glibc-2.24 support would be nice to get this landed to be a part of esr and aurora, there are no downsides to uplifting to beta and aurora at this point.
status-firefox52: --- → affected
status-firefox53: --- → affected
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b9a1da5e4f89
Status: UNCONFIRMED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Michal, Mike, want to request uplift to aurora/beta for this one?
Assignee: nobody → mgorny
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(mgorny)
(Assignee)

Comment 8

2 years ago
(In reply to Julien Cristau [:jcristau] from comment #7)
> Michal, Mike, want to request uplift to aurora/beta for this one?

Yes, please uplift it. It would certainly be helpful to have this fix around before we unmask new glibc on our users.
Flags: needinfo?(mh+mozilla)
Comment on attachment 8837466 [details] [diff] [review]
0001-Include-sys-sysmacros.h-for-major-minor-on-Linux.patch

Approval Request Comment
[Feature/Bug causing the regression]: Header changes in newer glibc
[User impact if declined]: Build failure against newer glibc (recent distros)
[Is this code covered by automated tests?]: N/A
[Has the fix been verified in Nightly?]: N/A
[Needs manual test from QE? If yes, steps to reproduce]: N/A
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: No
[Why is the change risky/not risky?]: Adds a #include of a header that has existed since the dawn of times, but happened to be #included indirectly, which is what ceases to happen with newer glibc.
[String changes made/needed]: N/A
Attachment #8837466 - Flags: approval-mozilla-beta?
Attachment #8837466 - Flags: approval-mozilla-aurora?
tracking-firefox52: ? → -
tracking-firefox53: ? → -
Comment on attachment 8837466 [details] [diff] [review]
0001-Include-sys-sysmacros.h-for-major-minor-on-Linux.patch

build fix for new glibc, aurora53+, beta52+

Let's get this in ESR52 so linux distros don't need to patch it in.
Attachment #8837466 - Flags: approval-mozilla-beta?
Attachment #8837466 - Flags: approval-mozilla-beta+
Attachment #8837466 - Flags: approval-mozilla-aurora?
Attachment #8837466 - Flags: approval-mozilla-aurora+
Flags: needinfo?(mgorny)

Comment 11

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/d4e8a7b1832d
status-firefox53: affected → fixed

Comment 12

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/5326967d9271
status-firefox52: affected → fixed

Comment 13

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/5326967d9271
status-firefox-esr52: --- → fixed
See Also: → bug 1406233
You need to log in before you can comment on or make changes to this bug.