Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aehm, sorry but there must be a better solution than this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is it that disturbs you here?
Is it the check for
drbg->pool != NULL
before assigningdrbg->pool = NULL
?would you prefer an unconditional assignment instead?
My rationale behind the extra check was that it ensures that the if-condition is practically always false unless an internal error occurred. So a write will never happen under normal circumstances.
Please also note the master drbg has locking enabled, which means that the calls to
rand_drbg_get_entropy()
are protected by thedrbg->lock
write lock. which is taken and released indrbg_add()
. These are the corresponding callstacks during the execution of RAND_add():There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. And I am also not concerned about locking.
It is the resource flow that happens in drbg_add.
The problem is the drbg->pool is not cleared, when res != 0, but the object is freed.
Therefore the pointer value becomes indeterminate by the C standard.
But later it is used in an if condition, here:
Using stale pointers in this way is very dangerous, and should be avoided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am using the
drbg->pool
just as boolean variable here to differentiate between the case where the pool data was attached to a buffer provided by RAND_add()/RAND_seed() or if it was regularly allocated by the get_entropy callback. In the former case I am not allowed to clear it, because the buffer pointers are declared const (but the constness had to be cast away).I can understand that a local pointer value might disappear because it was stored in a register and the register is now reused. But I wouldn't expect that the compiler explicitely erases the
drbg->pool
member. Or am I missing something?If you really insist, I would have to add a separate boolean variable
drbg->attached_pool
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a C standard issue. It says that pointers become "indeterminate" after the object
they refer to is destroyed with free, or goes out of scope. In the past we re-wrote code
in OpenSSL that used indeterminate pointer values.
Actually I think I don't like the way how the pointer is encapsulated in a pool, and the
pool is not free'd where it is allocated. I think the get_entropy function should
not free something that it did not own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I'll prepare a pull request (but not right away) and request your review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is perhaps helpful when thinking about the C standard to consider things like segmented addressing. If, after free(), the segment for the freed pointer becomes invalid, then even attempting to load the pointer value into a register (without dereferencing it) could cause a fault. (IIUC -- I don't think I've knowingly interacted with machines using segmented addressing!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good point.