Use signed integer instead of unsigned integer for the PRNG's size argument.

[ple13]

The functions GeneratePseudorandomBytes and GenerateUniformRandomRange takes the argument size of type uint64_t. This will cause a problem if a negative number is provided. For example, -1 will be implicitly converted to 2^64-1, and GeneratePseudorandomBytes(-1) will become GeneratePseudorandomBytes(2^64-1).

[wfa-reviewable]

This change is 

[SanjayVas]

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ple13)

---

src/main/cc/math/open_ssl_uniform_random_generator.h line 89 at r1 (raw file):

  // Generates a vector of pseudorandom bytes with the given size.
  absl::StatusOr<std::vector<unsigned char>> GeneratePseudorandomBytes(
      int64_t size) override;

Note that there are other tricks to avoiding implicit conversion¹ if we feel like using them. e.g. if we're willing to upgrade to C++20 like we have internally at Google, we can use std::same_as. Of course this PR is fine as-is assuming we're okay with limiting to sizes that fit into int64_t and having negative values as a runtime error.

---

src/test/cc/math/open_ssl_uniform_random_generator_test.cc line 84 at r1 (raw file):

      seq.status(),
      StatusIs(absl::StatusCode::kInvalidArgument,
               "Number of pseudorandom bytes must be a non-negative value."));

nit: avoid checking exact error messages. Error messages should not be considered part of the API contract. Use other string matchers such as HasSubstr with a minimal substring if the message is needed to resolve ambiguities between error conditions.

Suggestion:

      StatusIs(absl::StatusCode::kInvalidArgument,
               HasSubstr("negative")));

Footnotes

1. https://stackoverflow.com/questions/12877546/how-do-i-avoid-implicit-conversions-on-non-constructing-functions ↩

[ple13]

Reviewable status: 3 of 4 files reviewed, all discussions resolved (waiting on @SanjayVas)

---

src/main/cc/math/open_ssl_uniform_random_generator.h line 89 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Note that there are other tricks to avoiding implicit conversion¹ if we feel like using them. e.g. if we're willing to upgrade to C++20 like we have internally at Google, we can use std::same_as. Of course this PR is fine as-is assuming we're okay with limiting to sizes that fit into int64_t and having negative values as a runtime error.

Thanks.

---

src/test/cc/math/open_ssl_uniform_random_generator_test.cc line 84 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

nit: avoid checking exact error messages. Error messages should not be considered part of the API contract. Use other string matchers such as HasSubstr with a minimal substring if the message is needed to resolve ambiguities between error conditions.

Done.

Footnotes

1. https://stackoverflow.com/questions/12877546/how-do-i-avoid-implicit-conversions-on-non-constructing-functions ↩

[SanjayVas]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @stevenwarejones)

[stevenwarejones]

Reviewed 3 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ple13)