google / google/differential-privacy

Improving comments in privacy-critical code blocks.

Open
#16 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
3.4k
Forks
434
Avg merge
4d 9h
Merged PRs (30d)
9

Description

The lack of comments makes this code harder to audit. Consider https://github.com/google/differential-privacy/blob/master/differential_privacy/algorithms/rand.cc

It is not clear from reading the code what `SecureURBG::RefreshCache` does. Is it that you are computing a whole buffer of randomness at once and then feeding it out byte-by-byte? That is not a cache, then, but a buffer.

In `SecureURBG::result_type SecureURBG::operator`, it is not clear why `old_index` is copied from `current_index` before the `memcpy` operation:

```
SecureURBG::result_type SecureURBG::operator()() {
absl::WriterMutexLock lock(&mutex_);
if (current_index_ + sizeof(result_type) > kCacheSize) {
RefreshCache();
}
int old_index = current_index_;
current_index_ += sizeof(result_type);
result_type result;
std::memcpy(&result, cache_ + old_index, sizeof(result_type));
return result;
}
```
Why the extra copy, and not simply copy it like this:
```
SecureURBG::result_type SecureURBG::operator()() {
absl::WriterMutexLock lock(&mutex_);
if (current_index_ + sizeof(result_type) > kCacheSize) {
RefreshCache();
}
result_type result;
std::memcpy(&result, cache_ + current_index_, sizeof(result_type));
current_index_ += sizeof(result_type);
return result;
}
```

The programmer's intent is not clear from the C++ code, so it would be useful to have comments to explain it. It would also be nice to know what URBG stands for.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.