From 9326b4d131cebfdb2c3177c174e20ecff508ea79 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 23 Mar 2017 10:00:05 -0700
Subject: [PATCH] Fix some range issues and 32-bit issues with avatar
 generation

Summary:
Ref T12444. A few issues:

   - `x % (y - z)` doesn't generate values in the full range: the largest value is never generated. Instead, use `x % (1 + y - z)`.
   - `digestToRange(1, count)` never generates 0. After fixing the first bug, it could generate `count`. The range of the arrays is `0..(count-1)`, inclusive. Generate the correct range instead.
   - `unpack('L', ...)` can unpack a negative number on a 32-bit system. Use `& 0x7FFFFFFF` to mask off the sign bit so the result is always a positive integer.
   - FileFinder might return arbitrary keys, but we rely on sequential keys (0, 1, 2, ...)

Test Plan:
  - Used `bin/people profileimage ... --force` to regenerate images.
  - Added some debugging to verify that the math seemed to be working.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12444

Differential Revision: https://secure.phabricator.com/D17543
---
 .../builtin/PhabricatorFilesComposeAvatarBuiltinFile.php | 9 +++++----
 src/infrastructure/util/PhabricatorHash.php              | 5 +++--
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/src/applications/files/builtin/PhabricatorFilesComposeAvatarBuiltinFile.php b/src/applications/files/builtin/PhabricatorFilesComposeAvatarBuiltinFile.php
index 7eca716e34..4e461ec3f5 100644
--- a/src/applications/files/builtin/PhabricatorFilesComposeAvatarBuiltinFile.php
+++ b/src/applications/files/builtin/PhabricatorFilesComposeAvatarBuiltinFile.php
@@ -119,6 +119,7 @@ final class PhabricatorFilesComposeAvatarBuiltinFile
     foreach ($list as $file) {
       $map['alphanumeric/'.$file] = $root.$file;
     }
+
     return $map;
   }
 
@@ -138,11 +139,11 @@ final class PhabricatorFilesComposeAvatarBuiltinFile
     $border_seed = $username.'_border';
 
     $pack_key =
-      PhabricatorHash::digestToRange($pack_seed, 1, $pack_count);
+      PhabricatorHash::digestToRange($pack_seed, 0, $pack_count - 1);
     $color_key =
-      PhabricatorHash::digestToRange($color_seed, 1, $color_count);
+      PhabricatorHash::digestToRange($color_seed, 0, $color_count - 1);
     $border_key =
-      PhabricatorHash::digestToRange($border_seed, 1, $border_count);
+      PhabricatorHash::digestToRange($border_seed, 0, $border_count - 1);
 
     $pack = $pack_map[$pack_key];
     $icon = 'alphanumeric/'.$pack.'/'.$file.'.png';
@@ -188,7 +189,7 @@ final class PhabricatorFilesComposeAvatarBuiltinFile
       ->withFollowSymlinks(false)
       ->find();
 
-    return $map;
+    return array_values($map);
   }
 
   public static function getBorderMap() {
diff --git a/src/infrastructure/util/PhabricatorHash.php b/src/infrastructure/util/PhabricatorHash.php
index 7b5780460b..05b5fa719d 100644
--- a/src/infrastructure/util/PhabricatorHash.php
+++ b/src/infrastructure/util/PhabricatorHash.php
@@ -88,9 +88,10 @@ final class PhabricatorHash extends Phobject {
     }
 
     $hash = sha1($string, $raw_output = true);
-    $value = head(unpack('L', $hash));
+    // Make sure this ends up positive, even on 32-bit machines.
+    $value = head(unpack('L', $hash)) & 0x7FFFFFFF;
 
-    return $min + ($value % ($max - $min));
+    return $min + ($value % (1 + $max - $min));
   }