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

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[jvoisin]

Attachment: This patch is fixing the compilation issue

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.

Comment 1

[Frederik Braun [:freddy]]

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?

Comment 2

[Bob Owen (:bobowen)]

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

Comment 3

[Bob Owen (:bobowen)]

Attachment: Fix member variable initialisation order in LogMessage stub in Linux Sandbox.cpp

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

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Avoid warning-as-error sandbox build failure with an explicit cast.

I think this is the last warning-as-error fix we need: https://tbpl.mozilla.org/?tree=Try&rev=404754c9d255

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Fix namespace bug in Linux sandbox LOG_ERROR macro.

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

Comment 7

[Bob Owen (:bobowen)]

Thanks Jed,

I think jvoisin should be the assignee here as he found and fixed the original problem.

Comment 8

[jvoisin]

Fine for me. What do I need to do to get it merged?

Comment 9

[Guillaume Destuynder [:kang] [this account is no longer active]]

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

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=404754c9d255 (from comment #5).

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a2dde11f9906
https://hg.mozilla.org/integration/mozilla-inbound/rev/78ebcfff1234
https://hg.mozilla.org/integration/mozilla-inbound/rev/9362c5739379

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a2dde11f9906
https://hg.mozilla.org/mozilla-central/rev/78ebcfff1234
https://hg.mozilla.org/mozilla-central/rev/9362c5739379