Const data tables should go in a read-only data section (security/)

RESOLVED FIXED in Firefox 54

Status

()

Core
Security: PSM
P1
normal
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: dmajor, Assigned: dmajor)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [psm-assigned])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

10 months ago
Please see bug 1334254 comment 0 for background.

The various data tables under security/ are our largest source of const-related codegen issues. Fixing these files removes 220k of initializers from xul.dll, and moves an additional 96k of data from writable memory to the read-only section.

Before:
Size of xul.dll is 67.457536 MB
      name:   mem size  ,  disk size
     .text: 42.462635 MB
    .rdata:  9.600278 MB
     .data:  1.464016 MB,  1.017856 MB

After:
Size of xul.dll is 67.237888 MB
      name:   mem size  ,  disk size
     .text: 42.158107 MB
    .rdata:  9.696390 MB
     .data:  1.367664 MB,  1.005568 MB
(Assignee)

Comment 1

10 months ago
Created attachment 8831920 [details] [diff] [review]
patch

All but one of these changes are upgrading "const" to "constexpr" in order to make Visual Studio do the right thing.

In one case, ExtendedValidation.cpp, I couldn't make the table constexpr due to its mutable oid_tag field. Instead I removed "const" from ev_root_sha256_fingerprint, as removing the inner "const" is another way to work around the Visual Studio issue.

A number of these changes are to generated files. I've included both the tool change and the resulting code file, except in the case of the generated HSTS and HKPK files, which I will leave to the auto-update machinery. Let me know if I should take a different approach.
Assignee: nobody → dmajor
Attachment #8831920 - Flags: review?(dkeeler)
Comment on attachment 8831920 [details] [diff] [review]
patch

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

This looks great. For the EV info array, I imagine we could have two arrays - one with constexpr data and the other with the (non-const) OID tags (where the idea is you match indices to go from one to the other). Would you be interested in making that change as well, or should we just do that as a follow-up sometime later?
In any case, thanks!
Attachment #8831920 - Flags: review?(dkeeler) → review+
Priority: -- → P1
Whiteboard: [psm-assigned]
(Assignee)

Comment 3

10 months ago
Great, thanks. I had considered the parallel arrays and worried it would be shot down as too much contortion just to please the compiler. I'm happy to take it on as a followup though.

Comment 4

10 months ago
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b03c9f4ac1b0
Add constexpr to data tables under security/ for better codegen on Windows. r=keeler
(Assignee)

Updated

10 months ago
Blocks: 1335632

Comment 5

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b03c9f4ac1b0
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Comment 6

10 months ago
The next time we change the format of the data here, can you please give releng a heads up? This change broke a check we have to make sure the pins are up to date (bug 1335820).

Alternatively, if we could have a way to get the pin expiry date without relying on pattern matching, that would be even better :)

Comment 7

10 months ago
Backout by philringnalda@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/f985243bb630
Backed out changeset b03c9f4ac1b0 for Windows PGO bustage

Updated

10 months ago
Depends on: 1335981

Updated

10 months ago
Status: RESOLVED → REOPENED
status-firefox54: fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla54 → ---
(Assignee)

Comment 8

10 months ago
Just a quick note to say that this is still on my radar. I'll try reworking these patches in the hacky way (removing the inner const on the struct fields, but keeping the arrays as const), but I haven't gotten around to it yet.
(Assignee)

Comment 9

10 months ago
Created attachment 8835863 [details] [diff] [review]
Second attempt: remove inner const

This gets the same .text and .data savings as before, but in a more compiler-friendly way.
Attachment #8831920 - Attachment is obsolete: true
Attachment #8835863 - Flags: review?(dkeeler)
Comment on attachment 8835863 [details] [diff] [review]
Second attempt: remove inner const

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

As far as I understand, we're going this route because of a compiler bug that breaks the PGO builds? Maybe it would be best to file a undo-this-in-the-future bug and/or document why we're doing this rather than the better thing. If that's the case, it's unfortunate we have to do this since we're losing the suspenders in our belt-and-suspenders approach to making sure we don't accidentally change a field on one of these structures while we're running. For example, after this patch, someone could accidentally write:

for (CTLogInfo log : kCTLogList) {
  ...
  if (log.operatorIndex = 5) { // oops

  }
  ...
}

whereas before they would not have been able to (it *should* warn, and it shouldn't compile if we have warnings-as-errors on, but this is worse than the compiler refusing to compile it because of statically breaking constness).
In any case, It's fairly easy to check by hand that no one is doing the wrong thing right now, and it looks like only PublicKeyPinningService is (with TransportSecurityPreload - I filed bug 1338701 to fix it). Everything else looks fine.
Given all that, I'm still concerned about the EV information, since the array actually *isn't* const (because we have to set the OID field dynamically). If we still do the follow-up bug of separating that information out, can we make the array and every field in nsMyTrustedEVInfo const?

r=me if that all sounds good.
Attachment #8835863 - Flags: review?(dkeeler) → review+
(Assignee)

Comment 11

9 months ago
That's a fair point; I hadn't considered the range-for example. I guess in my mind I still think in terms of indices and iterators.

So, one option I was considering earlier was to use a type alias like:
#if defined(_MSC_VER) [and maybe also !defined(DEBUG)?]
template<typename T>
using logically_const = T;
#else
template<typename T>
using logically_const = const T;
#endif

That way one of the other platforms could still complain. I abandoned the idea mostly due to administrative concerns (where would it live, MFBT? argh, so much work for a hack!) but I'm open to being convinced to revive it.

Or, yeah, I'm also happy to just file a future-undo bug.

Regarding the EV infos, once the OID field is removed, the array could become const but the fields themselves would still have to be non-const for the same reason as in this bug.
Flags: needinfo?(dkeeler)
(In reply to David Major [:dmajor] from comment #11)
> That's a fair point; I hadn't considered the range-for example. I guess in
> my mind I still think in terms of indices and iterators.
> 
> So, one option I was considering earlier was to use a type alias like:
> #if defined(_MSC_VER) [and maybe also !defined(DEBUG)?]
> template<typename T>
> using logically_const = T;
> #else
> template<typename T>
> using logically_const = const T;
> #endif
> 
> That way one of the other platforms could still complain. I abandoned the
> idea mostly due to administrative concerns (where would it live, MFBT? argh,
> so much work for a hack!) but I'm open to being convinced to revive it.
> 
> Or, yeah, I'm also happy to just file a future-undo bug.

I think a future undo bug is fine, as long as we reference it in the code. I'd rather avoid jumping through type alias hoops if it's just for this one area of code.

> Regarding the EV infos, once the OID field is removed, the array could
> become const but the fields themselves would still have to be non-const for
> the same reason as in this bug.

Oh, right - of course.
Flags: needinfo?(dkeeler)
(Assignee)

Updated

9 months ago
Blocks: 1338873
(Assignee)

Comment 13

9 months ago
Equip for +2 defense against backout:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e523654411c1127928f35675854a20d438d2e138
https://treeherder.mozilla.org/#/jobs?repo=try&revision=29e5923a6598946ddead47365a02e5444d816484

Comment 14

9 months ago
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0c5a47e6516
Remove const from data tables under security/ for better codegen on Windows. r=keeler

Comment 15

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c0c5a47e6516
Status: REOPENED → RESOLVED
Last Resolved: 10 months ago9 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.