Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GMP] getuint256 crash fix. #829

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@furszy
Copy link
Collaborator

furszy commented Mar 6, 2019

Contains part of the complete mitigation for the underling issue on #828.

Illegal allocation of long serials crashes the node using gmp, openssl implementation only takes last 256 bits of those. This PR equalize getuint256 method behavior for those specific scenarios.

The chain will sync fine with gmp or openssl indifferently with this.

@wafflebot wafflebot bot added the review label Mar 6, 2019

@Warrows
Copy link
Collaborator

Warrows left a comment

concept ACK. These changes make sense and from what I can see they fix the crash. However I have some nit to suggest.

for (unsigned int i = 0, j = 0; i < sizeof(n); i++, j++)
((unsigned char*)&n)[i] = vch[j];
return n;
}else{

This comment has been minimized.

@Warrows

Warrows Mar 6, 2019

Collaborator

nit: get rid of the else statement, there is a return at the end of the if.

if(size > 32){
std::vector<unsigned char> vch(size);
mpz_export(&vch[0], NULL, -1, 1, 0, 0, bn);
uint256 n = 0;

This comment has been minimized.

@Warrows

Warrows Mar 6, 2019

Collaborator

nit: move the initialization of n before the if and change it to uint2556 n = uint256();

((unsigned char*)&n)[i] = vch[j];
return n;
}else{
uint256 n = 0;

This comment has been minimized.

@Warrows

Warrows Mar 6, 2019

Collaborator

nit: remove this initialization and do it above the if.

return n;
size_t size = (mpz_sizeinbase (bn, 2) + CHAR_BIT-1) / CHAR_BIT;
// This is just a fall back for invalid numbers, it will only take the last 256 bits as openssl implementation does.
if(size > 32){

This comment has been minimized.

@Warrows

Warrows Mar 6, 2019

Collaborator

nit: I think to remember the coding guidelines ask for a space between the closing parenthesis and the opening bracket.

@presstab

This comment has been minimized.

Copy link
Collaborator

presstab commented Mar 6, 2019

utACK. Nice work @furszy . Do we have any way to incorporate unit tests that would switch back and forth between openssl and gmp? Seems like that could be a good step in trying to discover any similar problems.

@furszy

This comment has been minimized.

Copy link
Collaborator Author

furszy commented Mar 6, 2019

yeah, talked about it with @Warrows today. He have coded the gmp/openssl files division, would be a good add on top of PR #744 .

Meanwhile, need to finish a new PR with some descendants problems of this nasty bug fixed first.

@furszy furszy closed this Mar 7, 2019

@wafflebot wafflebot bot removed the review label Mar 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.