Closed Bug 1335294 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Security: PSM, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: dmajor, Assigned: dmajor)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file, 1 obsolete file)

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
Attached patch patch (obsolete) — Splinter Review
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]
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.
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
Blocks: 1335632
https://hg.mozilla.org/mozilla-central/rev/b03c9f4ac1b0
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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 :)
Backout by philringnalda@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/f985243bb630
Backed out changeset b03c9f4ac1b0 for Windows PGO bustage
Depends on: 1335981
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla54 → ---
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.
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+
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)
Blocks: 1338873
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
https://hg.mozilla.org/mozilla-central/rev/c0c5a47e6516
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.