Build with "ac_add_options --enable-content-sandbox" is not compiling on Linux

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: julien.voisin, Assigned: julien.voisin)

Tracking

30 Branch
mozilla33
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140608211622

Steps to reproduce:

hg clone https://hg.mozilla.org/mozilla-central
echo "ac_add_options --enable-content-sandbox" > mozconfig
./mach build


Actual results:

1:28.44 SandboxAssembler.o
 1:28.81 In file included from ../../../dist/system_wrappers/prlog.h:3:0,
 1:28.81                  from /opt/mozilla-central/security/sandbox/linux/Sandbox.cpp:47:
 1:28.81 /opt/mozilla-central/security/sandbox/linux/Sandbox.cpp: In static member function ‘static void sandbox::Die::SandboxDie(const char*, const char*, int)’:
 1:28.81 /opt/mozilla-central/security/sandbox/linux/Sandbox.cpp:58:35: error: ‘gSeccompSandboxLog’ was not declared in this scope
 1:28.81  #define LOG_ERROR(args...) PR_LOG(gSeccompSandboxLog, PR_LOG_ERROR, (args))


Expected results:

A successful compilation.
Attachment #8452268 - Flags: review?(fbraun)
I haven't worked on sandbox, according to 'hg blame', :kang or :jld could be of help.
Can you guys take a look at this patch?
Flags: needinfo?(jld)
Flags: needinfo?(gdestuynder)
Attachment #8452268 - Flags: review?(fbraun)
Component: Untriaged → Security
Flags: needinfo?(jld)
Product: Firefox → Core
As it happens, after my conversation with jvoisin on IRC, I needed to compile this locally on linux while testing bug 1035275.

I hit the same error and this patch certainly allows the compile to complete.
Whether it's the best fix, I'm not sure.

On try we also appear to be treating some warnings as errors on some builds (that my local build doesn't).

Here are try pushes without and with this patch (there are patches from other bugs I'm working on in there, but I don't think they're affecting this compile):
https://tbpl.mozilla.org/?tree=Try&rev=499820e1b2dd
https://tbpl.mozilla.org/?tree=Try&rev=15865541acab
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: --enable-content-sanbox is not compiling → Build with "ac_add_options --enable-content-sandbox" is not compiling on Linux
Here's another patch to fix the member variable initialisation order, still getting other problems on Try though:
https://tbpl.mozilla.org/?tree=Try&rev=1e4f49c1fab3
Assignee: nobody → bobowencode
(In reply to Bob Owen (:bobowen) from comment #3)
> Here's another patch to fix the member variable initialisation order, still
> getting other problems on Try though:
> https://tbpl.mozilla.org/?tree=Try&rev=1e4f49c1fab3

Explicitly casting getpid() should be safe, because the value won't be negative.

For the missing syscalls on non-x86_64 architectures, I have a patch that should take care of it; I'll file a separate bug.
I've converted the diff to Mercurial export/import format so it can be checked in[*] — I added a commit message and filled in the author information from your bugzilla user info.  If you'd like something different there, please feel free to edit the patch and reupload it.

[*] https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Attachment #8452268 - Attachment is obsolete: true
Attachment #8454208 - Flags: review+
Attachment #8454208 - Flags: feedback?(julien.voisin)
Thanks Jed,

I think jvoisin should be the assignee here as he found and fixed the original problem.
Assignee: bobowencode → julien.voisin
(Assignee)

Comment 8

5 years ago
Fine for me. What do I need to do to get it merged?
(Assignee)

Updated

5 years ago
Attachment #8454208 - Flags: feedback?(julien.voisin) → feedback+
Comment on attachment 8454200 [details] [diff] [review]
Avoid warning-as-error sandbox build failure with an explicit cast.

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

Note that while is this safe because maxpid defaults to 32768 and is always 0 or positive, IIRC pid_t is a signed 32bit integer (/usr/include/bits/typesizes.h for ex.)
Attachment #8454200 - Flags: review?(gdestuynder) → review+
You need to log in before you can comment on or make changes to this bug.