Page MenuHomePhabricator

Fix logic of memory_cleanse() on MSVC and clean up docs
ClosedPublic

Authored by PiRK on Nov 12 2020, 13:20.

Details

Reviewers
majcosta
Group Reviewers
Restricted Project
Commits
rABCa0384bda1eb9: Fix logic of memory_cleanse() on MSVC and clean up docs
Summary

https://github.com/bitcoin/bitcoin/pull/16158/commits/cac30a436cab3641bba3b774d3d3ddbc426e7908

Commit fbf327b13868861c2877c5754caf5a9816f2603c ("Minimal code
changes to allow msvc compilation.") was indeed minimal in terms
of lines touched. But as a result of that minimalism it changed the
logic in memory_cleanse() to first call std::memset() and then
additionally the MSVC-specific SecureZeroMemory() function, and it
also moved a comment to the wrong location.
This commit removes the superfluous call to std::memset() on MSVC
and ensures that the comment is in the right position again.

https://github.com/bitcoin/bitcoin/pull/16158/commits/f53a70ce95231d34bb14cd6c58503927e8d7ff59

So far, the documentation of memory_cleanse() is a verbatim copy of
the commit message in BoringSSL, where this code was originally
written. However, our code evolved since then, and the commit message
is not particularly helpful in the code but is rather of historical
interested in BoringSSL only.

This commit improves improves the comments around memory_cleanse()
and gives a better rationale for the method that we use. This commit
touches only comments.

This is a backport of Core PR16158

Test Plan

ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK requested review of this revision.Nov 12 2020, 13:20
majcosta requested changes to this revision.Nov 13 2020, 05:06
majcosta added a subscriber: majcosta.

D8379 was (partly) missing, can you rebase?

This revision now requires changes to proceed.Nov 13 2020, 05:06

rebase on top of master after D8379 is landed. This is now a simple backport of PR16158

PiRK retitled this revision from Allow msvc compilation and improve documentation of memory_cleanse() to Fix logic of memory_cleanse() on MSVC and clean up docs.Nov 13 2020, 06:02
PiRK edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Nov 13 2020, 06:10