Closed Bug 1255425 Opened 8 years ago Closed 8 years ago

use a more efficient format for kSTSPreloadList

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(2 files, 2 obsolete files)

Entries in kSTSPreloadList currently look like:

class nsSTSPreload
{
  public:
    const char *mHost;
    const bool mIncludeSubdomains;
};

This is inefficient for a couple of reasons:

* The structure has a bunch of wasted space: it takes 8 bytes on 32-bit platforms and 16 bytes on 64-bit platforms, even though it only uses 5 and 9 bytes, respectively.

* The |const char*| requires additional space in the form of relocations (at least on Linux/OS X/Android), which double the space cost of individual entries.  (The space cost of the relocations is mitigating somewhat on Linux and Android because of elfhack, but there's still extra cost in the on-disk format and during the load of libxul to process those relocations.)

* The relocations the structure requires means that the data in it can't be shared between processes, which is important for e10s with multiple content processes.

We can make it more efficient by structuring it like so:

static const char kSTSPreloadHosts[] = {
  // One giant character array containing the hosts, in order:
  //   "0.me.uk\0007ssascha.de\00513c.com..."
};

struct nsSTSPreload
{
  // An index into kSTSPreloadHosts.
  uint32_t mHostIndex: 31;
  // We use the same datatype for both members so that MSVC will pack
  // the bitfields into a single uint32_t.
  uint32_t mIncludeSubdomains: 1;
};

nsSTSPreload now has no wasted space and is significantly smaller, especially on 64-bit platforms (saves ~29K on 32-bit platforms and ~85K on 64-bit platforms).  This organization does add a couple extra operations to searching for preload list entries, depending on your platform, but the space savings make it worth it.
The main loop of |output| tweaks entries, filters out entries based on
some conditions, and writes out the actual entries we're going to use.
Let's separate those three steps so it's clearer what's happening where.
Attachment #8730289 - Flags: review?(dkeeler)
This patch realizes the design described in comment 0.  Apologies for the
intertwingled parts, but I did not see a great way to separate things for
easier reviewing.

I verified as far as I could that:

* Running this script with the old .inc file (and without fetching new HSTS
  information) produces the same information in the new format; and

* Running this script with the new .inc file (and without fetching new HSTS
  information) produces the same information as running the script with the
  old .inc file.
Attachment #8730292 - Flags: review?(dkeeler)
Comment on attachment 8730292 [details] [diff] [review]
part 2 - pack kSTSPreloadList into a more efficient format

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

Just as a heads up, the changes in the two patches will result in failed ESLint checks:

> ./mach eslint security/
[...]
/home/m-i_drive/mozilla-inbound/security/manager/tools/getHSTSPreloadList.js
  282:12  error  Unexpected 'else' after 'return'  no-else-return
  294:14  error  "status" is already defined       no-redeclare
  304:14  error  "status" is already defined       no-redeclare
  313:14  error  "status" is already defined       no-redeclare
Comment on attachment 8730289 [details] [diff] [review]
part 1 - clearly delineate steps when outputting HSTS preload list

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

LGTM. Just the one comment, really (also, the lint issues should be addressed).

::: security/manager/tools/getHSTSPreloadList.js
@@ +279,1 @@
>        if (status.maxAge >= MINIMUM_REQUIRED_MAX_AGE || status.forceInclude) {

Logically, I think we should actually flip this (as in, check if we wouldn't want to include the site and return false early in that case).

@@ +286,2 @@
>        }
>        else {

Looks like we don't need this else anymore (superfluous else-after-return).
Oh, I guess the linter caught this - cool.
Attachment #8730289 - Flags: review?(dkeeler) → review+
Comment on attachment 8730292 [details] [diff] [review]
part 2 - pack kSTSPreloadList into a more efficient format

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

This is neat. I have a few comments but overall this looks good to me. About the (informative and helpful) commit message itself: I imagine "The space cost of the relocations is mitigating somewhat" should be "... mitigated"? Also, it's a bit confusing that the first hosts in kSTSPreloadHosts start with "0"s - maybe use "example.com\0example.org\0example.test..." or similar instead?

::: security/manager/tools/getHSTSPreloadList.js
@@ +40,5 @@
>  "/* nsSiteSecurityService.cpp, you shouldn't be #including it.     */\n" +
>  "/*****************************************************************************/\n" +
>  "\n" +
>  "#include <stdint.h>\n";
>  const PREFIX = "\n" +

We should consider moving PREFIX and POSTFIX down to where they're actually used, I think. (Particularly since this patch adds similar prefix/postfix strings for another section but doesn't declare them up here.)

@@ +294,5 @@
> +    for (var status in includedStatuses) {
> +      let incSubdomainsBool = (status.forceInclude && status.error != ERROR_NONE
> +                               ? status.originalIncludeSubdomains
> +                               : status.includeSubdomains);
> +      status.includeSubdomains = incSubdomainsBool;

I'm concerned that overwriting includeSubdomains in this way will cause confusion in the future. Maybe we could introduce another property newIncludeSubdomains or includeSubdomainsOut or something that is the result of this determination. Then, when writing out, that property is used.

@@ +297,5 @@
> +                               : status.includeSubdomains);
> +      status.includeSubdomains = incSubdomainsBool;
> +    }
> +
> +    writeTo("\nstatic const char kHSTSHostTable[] =\n", fos);

nit: while HSTS is probably more accurate, the preexisting data structures and types in this implementation use STS. We should be consistent one way or another.

@@ +304,5 @@
>      for (var status of includedStatuses) {
> +      indices[status.name] = currentIndex;
> +      // Add 1 for the null terminator in C.
> +      currentIndex += status.name.length + 1;
> +      writeTo("  \"" + status.name + "\\0\" /* " + (status.includeSubdomains ? "true" : "false") + " */\n", fos);

nit: break up long line

@@ +306,5 @@
> +      // Add 1 for the null terminator in C.
> +      currentIndex += status.name.length + 1;
> +      writeTo("  \"" + status.name + "\\0\" /* " + (status.includeSubdomains ? "true" : "false") + " */\n", fos);
> +    }
> +    writeTo(";\n\n", fos);

nit: let's just have one blank line added here

@@ +376,5 @@
>    var fis = Cc["@mozilla.org/network/file-input-stream;1"]
>                .createInstance(Ci.nsILineInputStream);
>    fis.init(file, -1, -1, Ci.nsIFileInputStream.CLOSE_ON_EOF);
>    var line = {};
> +  var v1EntryRegex = /  { "([^"]*)", (true|false) },/;

Might be nice to have a comment here explaining that this is so the script can read in both the previous format and the new format.
Attachment #8730292 - Flags: review?(dkeeler) → review+
There have been enough changes requested, and it was necessary to change how
the string table was output, that it seems reasonable to ask for another
review.  I think I addressed everything from the first review.  I haven't yet
tested on Windows, but I have confirmed that compiling a character array
initialized in the same way as the patch works fine on my Windows machine,
whereas the string concatentation version fails.
Attachment #8734487 - Flags: review?(dkeeler)
Attachment #8730292 - Attachment is obsolete: true
This required some changes for linting and the condition rearrangement;
probably worth a second pair of eyes on the condition rearrangement just to
make sure I didn't screw anything up.
Attachment #8734489 - Flags: review?(dkeeler)
Attachment #8730289 - Attachment is obsolete: true
Comment on attachment 8734489 [details] [diff] [review]
part 1 - clearly delineate steps when outputting HSTS preload list

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

Just the one issue - otherwise looks correct.

::: security/manager/tools/getHSTSPreloadList.js
@@ +277,4 @@
>  
> +    // Filter out entries we aren't including.
> +    var includedStatuses = sortedStatuses.filter(function (status) {
> +      if (status.maxAge <= MINIMUM_REQUIRED_MAX_AGE && !status.forceInclude) {

This comparison should be < rather than <=, I believe.
Attachment #8734489 - Flags: review?(dkeeler) → review+
Comment on attachment 8734487 [details] [diff] [review]
part 2 - pack kSTSPreloadList into a more efficient format

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

LGTM. You might mention in the commit message that we ended up having to write out each character rather than relying on string concatenation.
Attachment #8734487 - Flags: review?(dkeeler) → review+
(In reply to David Keeler [:keeler] (use needinfo?) from comment #8)
> ::: security/manager/tools/getHSTSPreloadList.js
> @@ +277,4 @@
> >  
> > +    // Filter out entries we aren't including.
> > +    var includedStatuses = sortedStatuses.filter(function (status) {
> > +      if (status.maxAge <= MINIMUM_REQUIRED_MAX_AGE && !status.forceInclude) {
> 
> This comparison should be < rather than <=, I believe.

Indeed, thank you for catching that!
https://hg.mozilla.org/mozilla-central/rev/f3339e6a451e
https://hg.mozilla.org/mozilla-central/rev/517f25117d11
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.