Closed Bug 1052033 Opened 6 years ago Closed 6 years ago

Fix warnings in security/sandbox and mark as FAIL_ON_WARNINGS

Categories

(Core :: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox33 --- unaffected
firefox34 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Fix clang warnings on OS X:

> security/sandbox/mac/Sandbox.mm:42:7 [-Wlogical-not-parentheses] logical not is only applied to the left hand side of this comparison                    
> security/sandbox/mac/Sandbox.mm:63:14 [-Wformat-security] format string is not a string literal (potentially insecure)

Suppress MSVC (false-positive) warning in third-party Chromium code:

> security\sandbox\chromium\base\shim\base\strings\string_piece.h(15) : warning C4717: '`anonymous namespace'::IsStringASCII' : recursive on all control paths, function will cause runtime stack overflow

Suppress MSVC warning in Microsoft header file about DLL exports. TBH, I'm not sure how serious this warning is but we have suppressed it in other bugs like bug 556886:

> c:\tools\msvs10\vc\include\typeinfo(157) : warning C4275: non dll-interface class 'stdext::exception' used as base for dll-interface class 'std::bad_cast'
Attachment #8470980 - Flags: review?(smichaud)
Assignee: nobody → cpeterson
Status: NEW → ASSIGNED
(Nit: This should be "depends on" bug 557566 (the bug that added FAIL_ON_WARNINGS support), not blocking it.

If we had a meta-bug tracking "add FAIL_ON_WARNINGS to directories", like the metabug for "fix build warnings", then it would make sense to have this block *that* bug. But bug 557566 is not that bug -- it's where we added the ability to annotate directories.  And bugs like this one intuitively *depend* on that feature; they don't block it.)
No longer blocks: FAIL_ON_WARNINGS
Depends on: FAIL_ON_WARNINGS
Comment on attachment 8470980 [details] [diff] [review]
fix-sandbox-warnings.patch

This looks fine to me.

-  if (!aInfo.type == MacSandboxType_Plugin) {
+  if (aInfo.type != MacSandboxType_Plugin) {

Thanks especially for catching this!  It's rather embarrassing :-(
Attachment #8470980 - Flags: review?(smichaud) → review+
https://hg.mozilla.org/mozilla-central/rev/953635c5e08b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1055541
You need to log in before you can comment on or make changes to this bug.