Skip to content

Improving comments in privacy-critical code blocks. #16

Open
@simsong

Description

@simsong

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions