bcgit / pc-dart

Pointy Castle - Dart Derived Bouncy Castle APIs
MIT License
237 stars 122 forks source link

class [Register64] may have wrong result on web platform #128

Closed licy183 closed 3 years ago

licy183 commented 3 years ago

I tried to implement argon2 on web platform, using the Register64 class in our package and got some error. So I have reviewed the code of Register64 class in src/ufixnum.dart, the Register64.mul function states the code following.

void mul(dynamic y) {
    if (y is int) {
      final lo32 = _lo32 * y;
      final carry = (lo32 ~/
          0x100000000); // TODO: use shift right when bug 17715 is fixed
      final hi32 = clip32(_hi32 * y) + carry;

      _hi32 = clip32(hi32);
      _lo32 = clip32(lo32);
    } else {
      final lo32 = _lo32 * y._lo32;
      final carry = (lo32 ~/
          0x100000000); // TODO: use shift right when bug 17715 is fixed
      final hi32 = clip32(_hi32 * y._lo32 as int) +
          clip32(_lo32 * y._hi32 as int) +
          carry;

      _hi32 = clip32(hi32);
      _lo32 = clip32(lo32 as int);
    }
  }

Dart2js will compile Dart int class as Javascript Number, which is implemented as double-precision floating-point format numbers according to the IEEE 754 standard, and can only safely represent integers between -(2^53 - 1) and 2^53 - 1. So the code above will have bad result. For example,

print(Register64(0xFFFFFFFF)..mul(0x200001))

will print 00200000ffdfffff on Dart VM but 00200000ffe00000 on Javascript.

Note that the dart team have fixnum package. In this package, a 64-bit integer (class Int64) is represented internally as three non-negative integers, storing the 22 low, 22 middle, and 20 high bits of the 64-bit value. But it is also an immutable implementation, so if we use it for computation operation, Dart VM/Javascript interpreter will create and destory large number of objects. So it may not be a good idea to replace current Register64 with Int64 in fixnum.

If I have some free time I will try to fix it and submit an PR.

Related Issues: #25

licy183 commented 3 years ago

PR: #130

licy183 commented 3 years ago

Merged. Close this Issue.