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

Fix Coverity CID:1453685 'unreachable code' in aes_xts code. #9902

Closed
wants to merge 2 commits into from

Conversation

@slontis
Copy link
Contributor

slontis commented Sep 15, 2019

Checklist
  • documentation is added or updated
  • tests are added or updated
@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 15, 2019

Hmmm, does this mean we need to update at least one Travis job to detect this? Although, I'm sure there are cases of dead code throughout our code, depending on configuration...

@slontis slontis added this to In progress in 3.0 New Core + FIPS via automation Sep 15, 2019
@slontis

This comment has been minimized.

Copy link
Contributor Author

slontis commented Sep 15, 2019

Hmmm, does this mean we need to update at least one Travis job to detect this? Although, I'm sure there are cases of dead code throughout our code, depending on configuration...

Should be ok. The code in here is platform independant, so it will get there.

@kroeckx

This comment has been minimized.

Copy link
Member

kroeckx commented Sep 15, 2019

Why isn't any of the existing test picking this up?

@kroeckx

This comment has been minimized.

Copy link
Member

kroeckx commented Sep 16, 2019

I don't agree this is ready to merge. I would like to understand why the test suite didn't pick this up. Maybe it doesn't have any tests covering XTS? Then it would be a good time to add them.

@slontis

This comment has been minimized.

Copy link
Contributor Author

slontis commented Sep 17, 2019

Maybe it doesn't have any tests covering XTS? Then it would be a good time to add them.

There are XTS tests in evp_test. (via evpciph.txt). The normal encrypt/decrypt update uses the cupdate method (see aes_xts_stream_update) - which calls the cipher method internally.
The only other way that the ccipher method can be called is via a call to EVP_Cipher().
@mattcaswell is this actually a useful interface (i.e EVP_Cipher) - if it is then it would need tests, because absolutely nothing hits this function currently.

@slontis

This comment has been minimized.

Copy link
Contributor Author

slontis commented Sep 17, 2019

I will update this PR to not also do the *outl = inl inside aes_ctx_stream_update since it is already done in aes_xts_cipher.

Does this address your concern @kroeckx ?

@kroeckx kroeckx removed the approval: done label Sep 17, 2019
@kroeckx

This comment has been minimized.

Copy link
Member

kroeckx commented Sep 17, 2019

I guess that makes sense, it would at least explain why the test suite passed. I assume that with the 2nd commit only the test suite failed?

3.0 New Core + FIPS automation moved this from In progress to Reviewer approved Sep 17, 2019
levitte pushed a commit that referenced this pull request Sep 18, 2019
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Kurt Roeckx <kurt@roeckx.be>
(Merged from #9902)
@slontis

This comment has been minimized.

Copy link
Contributor Author

slontis commented Sep 18, 2019

Thanks for reviewing. Merged to master.

@slontis slontis closed this Sep 18, 2019
3.0 New Core + FIPS automation moved this from Reviewer approved to Done Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4 participants
You can’t perform that action at this time.