1443342 - Various unsigned integer overflow fixups for the sanitizers

Status: RESOLVED FIXED

(Core :: MFBT, enhancement)

Description

[Jeff Walden [:Waldo]]

This is a bit grab-baggy, oh well.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Remove HashFunctions.h's RotateBitsLeft32 and use the general RotateLeft function instead

There doesn't seem to be any value at all to a length-specific function over a length-generic one.  Plus you pick up some more assertions along the way.  If explicitly specifying the bit length is desirable, we can throw a <uint32_t> into the RotateLeft call, but I don't think it's particularly useful.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Opt one HashFunctions.h function out of integer-overflow sanitizing and don't blacklist HashFunctions.h

It looks like this is the only function with unsigned overflows in it.  (Note that narrowing casts don't seem to trigger the unsigned-overflow sanitizer.  This could probably be argued either way with the sanitizer folks, but I'm just living with reality now.)

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Change nsZipArchive.cpp's HashName string-hashing implementation to mozilla::HashString

The current function has overflows in it, so either it has to be blacklisted, or we swap algorithms.  Swapping to a common one seems better to me.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad2de84f77437cc9f2134a605509695808d0d1ae doesn't seem to suggest any abstraction-violating assumptions occur with respect to this hash function (or issues with any of the rest of these patches).

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Remove XorShift128PlusRNG.h from integer-overflow sanitizer blacklisting

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Don't blacklist nsCSSProps.cpp:SortPropertyAndCount from integer-overflow sanitizing

The uint32_t field is limited by shorthand counts that are very small.  The enum, if you look up the definition, has a -1 initializer (so must be signed), so the secondary sort by property index doesn't have overflows in it.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 8956266 [details] [diff] [review]
Change nsZipArchive.cpp's HashName string-hashing implementation to mozilla::HashString

Hm, belay that for a sec.  Some hidden X orange in there that this change caused.  Oddment.

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Blacklist nsZipArchive.cpp's entry-name hashing function from being checked for unsigned integer overflows, because it knowingly relies on them

I think this function in AddonTestUtils.jsm:

  async manuallyInstall(xpiFile, installLocation = this.profileExtensions, id = null, unpacked = this.testUnpacked) {
    if (id == null) {
      id = await this.getIDFromExtension(xpiFile);
    }

    if (unpacked) {
      let dir = installLocation.clone();
      dir.append(id);
      dir.create(Ci.nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);

      let zip = ZipReader(xpiFile);
      let entries = zip.findEntries(null);
      while (entries.hasMore()) {
        let entry = entries.getNext();
        let target = dir.clone();
        for (let part of entry.split("/"))
          target.append(part);
        zip.extract(entry, target);
        target.permissions |= FileUtils.PERMS_FILE;
      }
      zip.close();

      return dir;
    }

    let target = installLocation.clone();
    target.append(`${id}.xpi`);
    xpiFile.copyTo(target.parent, target.leafName);
    return target;
  },

may depend on the hashing behavior performed by HashName.  |nsIZipReader.extract| will create the file if it doesn't exist, but it appears that internally, that function *doesn't* handle creating intermediate directories that don't exist.  (This is a vague guess from skimming |PR_OpenFile|'s *nix implementation.)  And it looks like maybe, in that case, a META-INF/ entry gets processed first now to create that intermediate directory, but with my change a META-INF/mozilla.rsa entry gets processed *first* and you get the missing-intermediate-directory problem.

I probably could make this all more resilient -- extract should probably (?) also create intermediate directories, or perhaps this algorithm should change -- but this rat hole is deep enough already, so just don't bother changing the hash algorithm.

I also considered using explicit wrapping operations in HashName, but this probably points out a need for a template class like https://doc.rust-lang.org/beta/std/num/struct.Wrapping.html here in addition to the explicit wrapping operations.  Not sure precisely when I might get to that, but probably sometime.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8956324 [details] [diff] [review]
Blacklist nsZipArchive.cpp's entry-name hashing function from being checked for unsigned integer overflows, because it knowingly relies on them

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

WFM.

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8956262 [details] [diff] [review]
Remove HashFunctions.h's RotateBitsLeft32 and use the general RotateLeft function instead

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

::: mfbt/HashFunctions.h
@@ -55,5 @@
>  #include "mozilla/WrappingOperations.h"
>  
>  #include <stdint.h>
>  
> -#ifdef __cplusplus

All those C users!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/958171171857
Remove HashFunctions.h's RotateBitsLeft32 and use the general RotateLeft function instead.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c03114535c2
Opt one HashFunctions.h function out of integer-overflow sanitizing and don't blacklist HashFunctions.h.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0a425ee1c3e
Blacklist nsZipArchive.cpp's entry-name hashing function from being checked for unsigned integer overflows, because it knowingly relies on them.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ac335714eb8
Remove XorShift128PlusRNG.h from integer-overflow sanitizer blacklisting.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/60164da23cf4
Don't blacklist nsCSSProps.cpp:SortPropertyAndCount from integer-overflow sanitizing.  r=froydnj

Comment 11

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/958171171857
https://hg.mozilla.org/mozilla-central/rev/6c03114535c2
https://hg.mozilla.org/mozilla-central/rev/f0a425ee1c3e
https://hg.mozilla.org/mozilla-central/rev/0ac335714eb8
https://hg.mozilla.org/mozilla-central/rev/60164da23cf4