differential-privacy icon indicating copy to clipboard operation
differential-privacy copied to clipboard

Improving comments in privacy-critical code blocks.

Open simsong opened this issue 5 years ago • 0 comments

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.

simsong avatar Jan 23 '20 03:01 simsong