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 build errors and warnings with no-cms, no-dh and no-ec [1.0.2] #6087

Closed
wants to merge 3 commits into from
Closed

Fix build errors and warnings with no-cms, no-dh and no-ec [1.0.2] #6087

wants to merge 3 commits into from

Conversation

levitte
Copy link
Member

@levitte levitte commented Apr 26, 2018

This is a takeover of #1269, on request

@levitte levitte added the branch: 1.0.2 Merge to OpenSSL_1_0_2-stable branch label Apr 26, 2018
@levitte levitte mentioned this pull request Apr 26, 2018
Closed
@mattcaswell mattcaswell added this to the 1.0.2 milestone Apr 26, 2018
richsalz
richsalz reviewed Apr 26, 2018
View reviewed changes
Copy link
Contributor

@richsalz richsalz left a comment

Choose a reason for hiding this comment

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

one question, but approved either way.

crypto/dh/dh.h Outdated
@@ -356,7 +356,9 @@ int DH_KDF_X9_42(unsigned char *out, size_t outlen,

/* KDF types */
# define EVP_PKEY_DH_KDF_NONE 1
# ifndef OPENSSL_NO_CMS
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure about this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I already commented on this in 1269:

Hmmm, we do exactly this in 1.1.0 and on... so for the sake of consistency, and also for those who use the presence of EVP_PKEY_DH_KDF_X9_42 to determine if the functionality is available at all, this makes sense.

However, seeing that this is 1.0.2, it might be a bit on the late side to make this kind of change.

@richsalz richsalz added the approval: done This pull request has the required number of approvals label Apr 26, 2018
Cristian Stoica added 3 commits April 26, 2018 23:20
@levitte
fix warning unused-but-set-variable 'nostrict' (no-dh and no-ec)
7833fe1 
This patch fixes the following warning when OpenSSL is configured with
no-dh and no-ec:

./Configure no-ec no-dh linux-x86_64

...
s3_lib.c:4231:9: warning: variable 'nostrict' set but not used [-Wunused-but-set-variable]
     int nostrict = 1;
         ^

CLA: trivial
Signed-off-by: Cristian Stoica <cristian.stoica@nxp.com>
@levitte
fix warning unused-but-set-variable 'alg_k' (no-dh and no-ec)
cda3b36 
This patch fixes the following warning when OpenSSL is configured with
no-dh and no-ec:

./Configure no-ec no-dh linux-x86_64

...
s3_lib.c: In function 'ssl3_get_req_cert_type':
s3_lib.c:4234:19: warning: variable 'alg_k' set but not used [-Wunused-but-set-variable]
     unsigned long alg_k;

CLA: trivial
Signed-off-by: Cristian Stoica <cristian.stoica@nxp.com>
@levitte
s_server: fix warnings unused-but-set-variable (no-dh)
d5ed258 
This patch fixes the following two warnings when OpenSSL is built with no-dh option:

s_server.c: In function 's_server_main':
s_server.c:1105:25: warning: variable 'no_dhe' set but not used [-Wunused-but-set-variable]
     int no_tmp_rsa = 0, no_dhe = 0, no_ecdhe = 0, nocert = 0;
                         ^
s_server.c:1101:11: warning: variable 'dhfile' set but not used [-Wunused-but-set-variable]
     char *dhfile = NULL;
           ^
CLA: trivial
Signed-off-by: Cristian Stoica <cristian.stoica@nxp.com>
@levitte
Copy link
Member Author

levitte commented Apr 26, 2018

Oh darn... the last commit isn't covered by a CLA, and isn't trivial. Meh...

@levitte
Copy link
Member Author

levitte commented Apr 26, 2018

... or well, considering it's an obvious copy-n-paste with some other word changed to cms, it might actually be regarded as trivial.

How should we handle this. @richsalz?

@richsalz
Copy link
Contributor

We should ask him to sign the CLA.
We can assume that since he says "tweaks by me" on code that was written by David Woodhouse, who has signed a CLA, that his part of the commit was trivial. It would be nice to verify that, if possible.
Safest thing is to revert that one commit and rewrite things.

@levitte levitte force-pushed the cristian-stoica-OpenSSL_1_0_2-stable branch from f147d44 to d5ed258 Compare April 27, 2018 03:59
levitte pushed a commit that referenced this pull request Apr 27, 2018
@levitte
fix warning unused-but-set-variable 'nostrict' (no-dh and no-ec)
76b8b69 
This patch fixes the following warning when OpenSSL is configured with
no-dh and no-ec:

./Configure no-ec no-dh linux-x86_64

...
s3_lib.c:4231:9: warning: variable 'nostrict' set but not used [-Wunused-but-set-variable]
     int nostrict = 1;
         ^

CLA: trivial
Signed-off-by: Cristian Stoica <cristian.stoica@nxp.com>

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #6087)
levitte pushed a commit that referenced this pull request Apr 27, 2018
@levitte
fix warning unused-but-set-variable 'alg_k' (no-dh and no-ec)
60ced07 
This patch fixes the following warning when OpenSSL is configured with
no-dh and no-ec:

./Configure no-ec no-dh linux-x86_64

...
s3_lib.c: In function 'ssl3_get_req_cert_type':
s3_lib.c:4234:19: warning: variable 'alg_k' set but not used [-Wunused-but-set-variable]
     unsigned long alg_k;

CLA: trivial
Signed-off-by: Cristian Stoica <cristian.stoica@nxp.com>

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #6087)
levitte pushed a commit that referenced this pull request Apr 27, 2018
@levitte
s_server: fix warnings unused-but-set-variable (no-dh)
b10794b 
This patch fixes the following two warnings when OpenSSL is built with no-dh option:

s_server.c: In function 's_server_main':
s_server.c:1105:25: warning: variable 'no_dhe' set but not used [-Wunused-but-set-variable]
     int no_tmp_rsa = 0, no_dhe = 0, no_ecdhe = 0, nocert = 0;
                         ^
s_server.c:1101:11: warning: variable 'dhfile' set but not used [-Wunused-but-set-variable]
     char *dhfile = NULL;
           ^
CLA: trivial
Signed-off-by: Cristian Stoica <cristian.stoica@nxp.com>

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #6087)
@levitte
Copy link
Member Author

levitte commented Apr 27, 2018

Ok, I removed that last commit and merged the rest.

b10794b s_server: fix warnings unused-but-set-variable (no-dh)
60ced07 fix warning unused-but-set-variable 'alg_k' (no-dh and no-ec)
76b8b69 fix warning unused-but-set-variable 'nostrict' (no-dh and no-ec)

@levitte levitte closed this Apr 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richsalz richsalz richsalz approved these changes

Assignees
No one assigned
Labels
approval: done This pull request has the required number of approvals branch: 1.0.2 Merge to OpenSSL_1_0_2-stable branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@levitte @richsalz @mattcaswell