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.

Sign up for GitHub

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

Jump to bottom

Fix unreachable code in cms_main() in apps #21474

Closed
wants to merge 3 commits into from
Closed

Fix unreachable code in cms_main() in apps #21474

wants to merge 3 commits into from

Conversation

nv-dmd
Copy link

@nv-dmd nv-dmd commented Jul 17, 2023

I suggest changing code so that there is no unreachable code.
There is a warning in cms_main() on line 843, but it is unreachable
because on line 817 there is a check 'if (operation == SMIME_ENCRYPT)', and line 843 is in this loop and condition to hit the line 843 is 'operation != SMIME_ENCRYPT'

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jul 17, 2023
@tom-cosgrove-arm
Copy link
Contributor

This looks like it could be accepted without a CLA. Please add CLA: trivial to the commit message on a line on its own.

Alternatively please submit a CLA as per https://www.openssl.org/policies/cla.html

@nv-dmd
Copy link
Author

nv-dmd commented Jul 17, 2023

This looks like it could be accepted without a CLA. Please add CLA: trivial to the commit message on a line on its own.

Alternatively please submit a CLA as per https://www.openssl.org/policies/cla.html

Of course, I can add the CLA:trivial, but I signed the ICLA at last week and I received an email confirming that "I've added you to our CLA database and you should now be able to contribute"

@nv-dmd nv-dmd force-pushed the unreachable_code_apps_cms_main branch from 0df4b80 to bdb75e5 Compare July 17, 2023 11:03
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Jul 17, 2023
@tom-cosgrove-arm
Copy link
Contributor

I signed the ICLA at last week and I received an email confirming that "I've added you to our CLA database and you should now be able to contribute"

Thanks - looks like the bot was looking at out-of-date data

@tom-cosgrove-arm tom-cosgrove-arm added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: cleanup The issue/pr deals with cleanup of comments/docs not altering code significantly tests: exempted The PR is exempt from requirements for testing labels Jul 17, 2023
tom-cosgrove-arm
tom-cosgrove-arm approved these changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tom-cosgrove-arm tom-cosgrove-arm removed the approval: review pending This pull request needs review by a committer label Jul 17, 2023
@paulidale
Copy link
Contributor

Of course, I can add the CLA:trivial, but I signed the ICLA at last week and I received an email confirming that "I've added you to our CLA database and you should now be able to contribute"

I suspect that there is an email address mismatch. Setting the author on the git commit might fix the CLA issue: https://www.git-tower.com/learn/git/faq/change-author-name-email

@t8m
Copy link
Member

t8m commented Jul 17, 2023

I do not think this is eligible for CLA: trivial. Please revert the change adding it and use the e-mail address you've provided in your ICLA when committing.

@nv-dmd nv-dmd force-pushed the unreachable_code_apps_cms_main branch from bdb75e5 to a26f089 Compare July 17, 2023 12:09
@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jul 17, 2023
@nv-dmd
Copy link
Author

nv-dmd commented Jul 17, 2023

I do not think this is eligible for CLA: trivial. Please revert the change adding it and use the e-mail address you've provided in your ICLA when committing.

I reverted the CLA: trivial in the commit.
But I use a.tishkov@aladdin.ru email address in my commits. I specified this address in ICLA and I received a email letter confirming that my email was added to the database in this email adress

@t8m t8m closed this Jul 17, 2023
@t8m t8m reopened this Jul 17, 2023
@t8m t8m closed this Jul 17, 2023
@t8m t8m reopened this Jul 17, 2023
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Jul 17, 2023
@t8m
Copy link
Member

t8m commented Jul 17, 2023

There was a problem with the letter case in your e-mail address in the CLA database. Fixed now.

t8m
t8m reviewed Jul 17, 2023
View reviewed changes
apps/cms.c Outdated Show resolved Hide resolved
@t8m t8m added the triaged: refactor The issue/pr requests/implements refactoring label Jul 17, 2023
tom-cosgrove-arm
tom-cosgrove-arm requested changes Jul 18, 2023
View reviewed changes
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now needs changes :(

@tom-cosgrove-arm tom-cosgrove-arm added the approval: review pending This pull request needs review by a committer label Jul 18, 2023
@t8m t8m added triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 and removed triaged: refactor The issue/pr requests/implements refactoring triaged: cleanup The issue/pr deals with cleanup of comments/docs not altering code significantly labels Jul 18, 2023
@t8m
Copy link
Member

t8m commented Jul 18, 2023

This actually fixes a bug - the warning was not produced so IMO this is eligible also for 3.1 and 3.0 branches.

@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Jul 18, 2023
tom-cosgrove-arm
tom-cosgrove-arm approved these changes Jul 18, 2023
View reviewed changes
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LTM

@tom-cosgrove-arm tom-cosgrove-arm added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jul 18, 2023
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jul 19, 2023
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@t8m
Copy link
Member

t8m commented Jul 19, 2023

Merged to master, 3.1, and 3.0 branches. Thank you for your contribution.

@t8m t8m closed this Jul 19, 2023
openssl-machine pushed a commit that referenced this pull request Jul 19, 2023
@t8m
apps/cms.c: Fix unreachable code in cms_main()
dfdc9e3 
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #21474)

(cherry picked from commit 8c34367)
openssl-machine pushed a commit that referenced this pull request Jul 19, 2023
@t8m
apps/cms.c: Fix unreachable code in cms_main()
8c34367 
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #21474)
openssl-machine pushed a commit that referenced this pull request Jul 19, 2023
@t8m
apps/cms.c: Fix unreachable code in cms_main()
866f43a 
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #21474)

(cherry picked from commit 8c34367)
@nv-dmd nv-dmd deleted the unreachable_code_apps_cms_main branch July 19, 2023 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8m t8m t8m approved these changes

@tom-cosgrove-arm tom-cosgrove-arm tom-cosgrove-arm approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nv-dmd @tom-cosgrove-arm @paulidale @t8m @openssl-machine