Closed Bug 1443342 Opened 2 years ago Closed 2 years ago

Various unsigned integer overflow fixups for the sanitizers

Categories

(Core :: MFBT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: Waldo, Assigned: Waldo)

Details

Attachments

(5 files, 1 obsolete file)

This is a bit grab-baggy, oh well.
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.
Attachment #8956262 - Flags: review?(nfroyd)
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.)
Attachment #8956263 - Flags: review?(nfroyd)
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).
Attachment #8956266 - Flags: review?(nfroyd)
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.
Attachment #8956268 - Flags: review?(nfroyd)
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.
Attachment #8956266 - Flags: review?(nfroyd)
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.
Attachment #8956324 - Flags: review?(nfroyd)
Attachment #8956266 - Attachment is obsolete: true
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.
Attachment #8956324 - Flags: review?(nfroyd) → review+
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!
Attachment #8956262 - Flags: review?(nfroyd) → review+
Attachment #8956263 - Flags: review?(nfroyd) → review+
Attachment #8956267 - Flags: review?(nfroyd) → review+
Attachment #8956268 - Flags: review?(nfroyd) → review+
Pushed by jwalden@mit.edu:
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
You need to log in before you can comment on or make changes to this bug.