crypto test coverage - remaining blockers in crypto

Hi.
I try to improve test coverage of internal/crypto.

Already I submitted pull requests that I can write test.
#17555, #17458, #17449, #17447, #17426, #17418, #17728 and #17730.

I found two coverage blockers.

1. `process.binding('crypto').PBKDF2` does not returns -1

Code

node/lib/internal/crypto/pbkdf2.js

Lines 82 to 87 in 7124b46

    
             if (PBKDF2(password, salt, iterations, keylen, digest, next) === -1) 
        
               throw new errors.TypeError('ERR_CRYPTO_INVALID_DIGEST', digest); 
        
           } else { 
        
             const ret = PBKDF2(password, salt, iterations, keylen, digest); 
        
             if (ret === -1) 
        
               throw new errors.TypeError('ERR_CRYPTO_INVALID_DIGEST', digest);

Reason

PBKDF2 returns undefined always because PBKDF2 returns anything.
See also:

node/src/node_crypto.cc

Line 5569 in efffcc2

void PBKDF2(const FunctionCallbackInfo<Value>& args) {

So never match returns -1.

2. Can't throw `ERR_CRYPTO_HASH_UPDATE_FAILED`

Code

node/lib/internal/crypto/hash.js

Lines 57 to 58 in 7907534

    
           if (!this._handle.update(data, encoding || getDefaultEncoding())) 
        
             throw new errors.Error('ERR_CRYPTO_HASH_UPDATE_FAILED');

Reason

Hash#_handle.update does not throw an Error(ERR_CRYPTO_HASH_UPDATE_FAILED).

Hash#_handle.update returns false when mdctx_ pointed at null.
But I could not reproduce mdctx_ make the null pointer.

See also:

node/src/node_crypto.cc

Lines 4131 to 4133 in efffcc2

    
           bool Hash::HashUpdate(const char* data, int len) { 
        
             if (mdctx_ == nullptr) 
        
               return false;

1: I think should remove check where PBKDF2 returns -1.
2: Please teach me how to make mdctx_ to the null pointer.

That if (ret === -1) check is simply wrong and can be removed. The synchronous version of PBKDF2() never returns that - it either returns a buffer or it throws an exception.

It shouldn't be possible to reach that if (mdctx_ == nullptr) branch. It can be removed or replaced with a CHECK_NE(mdctx_, nullptr).

Does code coverage see a CHECK as a branch without coverage? I'm guessing no because it's a termination point.

@bnoordhuis

Re: ret === -1, that is actually not correct. It returns -1 if an invalid digest is passed. See

node/src/node_crypto.cc

Lines 5598 to 5607 in efffcc2

    
           if (args[4]->IsString()) { 
        
             node::Utf8Value digest_name(env->isolate(), args[4]); 
        
             digest = EVP_get_digestbyname(*digest_name); 
        
             if (digest == nullptr) { 
        
               free(salt); 
        
               free(pass); 
        
               args.GetReturnValue().Set(-1); 
        
               return; 
        
             } 
        
           }

The original confusion here might come from the fact that C++ return values are not what gets returns to the JS land, those return values come from args.GetReturnValue().Set(value); calls.

Re: HashUpdate, I don't know this code nearly as well as @bnoordhuis but just following the logic there, it seems like one could reach the false state at the very least the following way:

node/src/node_crypto.cc

Lines 4149 to 4152 in efffcc2

    
           if (!decoder.Decode(env, args[0].As<String>(), args[1], UTF8)) { 
        
             args.GetReturnValue().Set(false); 
        
             return; 
        
           }

@Leko

Ah, so that was changed recently in 7124b46, before that it worked the way I described.

it seems like one could reach the false state at the very least the following way

That's not the line @Leko linked to, though. That line should be unreachable because it should never be possible to construct a Hash object that has mdctx_ == nullptr, the constructor should throw when initialization fails.

@Leko

That's not the line @Leko linked to, though. That line should be unreachable because it should never be possible to construct a Hash object that has mdctx_ == nullptr, the constructor should throw when initialization fails.

Sorry, I'm just responding to the original premise of "Can't throw ERR_CRYPTO_HASH_UPDATE_FAILED" which is definitely possible. Didn't mean to disagree re: it not being possible to reach via the route presented.

@bnoordhuis

@bnoordhuis @apapirovski
Thank you for your information.

C++ return values are not what gets returns to the JS land, those return values come from args.GetReturnValue().Set(value); calls.

I didn't know that. I'd misunderstood.

it seems like one could reach the false state at the very least the following way:

I'll try to reach this condition.

Navigation Menu

Repository navigation

crypto test coverage - remaining blockers in crypto #17731

1. `process.binding('crypto').PBKDF2` does not returns -1

Code

Reason

2. Can't throw `ERR_CRYPTO_HASH_UPDATE_FAILED`

Code

Reason

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	if (PBKDF2(password, salt, iterations, keylen, digest, next) === -1)
	throw new errors.TypeError('ERR_CRYPTO_INVALID_DIGEST', digest);
	} else {
	const ret = PBKDF2(password, salt, iterations, keylen, digest);
	if (ret === -1)
	throw new errors.TypeError('ERR_CRYPTO_INVALID_DIGEST', digest);

	if (!this._handle.update(data, encoding \|\| getDefaultEncoding()))
	throw new errors.Error('ERR_CRYPTO_HASH_UPDATE_FAILED');

	bool Hash::HashUpdate(const char* data, int len) {
	if (mdctx_ == nullptr)
	return false;

	if (args[4]->IsString()) {
	node::Utf8Value digest_name(env->isolate(), args[4]);
	digest = EVP_get_digestbyname(*digest_name);
	if (digest == nullptr) {
	free(salt);
	free(pass);
	args.GetReturnValue().Set(-1);
	return;
	}
	}

	if (!decoder.Decode(env, args[0].As<String>(), args[1], UTF8)) {
	args.GetReturnValue().Set(false);
	return;
	}

crypto test coverage - remaining blockers in crypto #17731

Description

1. process.binding('crypto').PBKDF2 does not returns -1

Code

Reason

2. Can't throw ERR_CRYPTO_HASH_UPDATE_FAILED

Code

Reason

Activity

bnoordhuis commented on Dec 18, 2017

apapirovski commented on Dec 18, 2017

bnoordhuis commented on Dec 18, 2017

apapirovski commented on Dec 18, 2017

Leko commented on Dec 21, 2017

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

1. `process.binding('crypto').PBKDF2` does not returns -1

2. Can't throw `ERR_CRYPTO_HASH_UPDATE_FAILED`