Skip to content

Comments

fix build breaks with nonstandard options#4518

Closed
kaduk wants to merge 2 commits intoopenssl:masterfrom
kaduk:noec
Closed

fix build breaks with nonstandard options#4518
kaduk wants to merge 2 commits intoopenssl:masterfrom
kaduk:noec

Conversation

@kaduk
Copy link
Contributor

@kaduk kaduk commented Oct 11, 2017

The no-ec automatic builder has been failing. While testing that fix, I ran into an issue specific to my gcc version's -O1 (-O0 or -O2 do not trigger it), so it's less clear that the second commit is worth including.

kaduk added 2 commits October 11, 2017 08:25
Now that we are moving to support named FFDH groups, these fields are not
ec-specific, so we need them to always be available.

This fixes the no-ec --strict-warnings build, since gcc
5.4.0-6ubuntu1~16.04.4 appears to always try to compile the static inline
functions from ssl_locl.h, even when they are not used in the current
compilation unit.
test/bad_dtls_test.c: In function 'validate_client_hello':
test/bad_dtls_test.c:128:33: error: 'u' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     if (!PACKET_get_1(&pkt, &u) || u != SSL3_RT_HANDSHAKE)
                                 ^
Apparently -O1 does not perform sufficient optimization to ascertain
that PACKET_get_1 will always initialize u if it returns true.
@richsalz
Copy link
Contributor

This fixes a build break and the second commit is certainly stuff we've done before to appease compilers. So while I don't like to approve co-workers stuff, the build-break is more important. +1

Copy link
Member

@levitte levitte left a comment

Choose a reason for hiding this comment

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

(does this put you more at ease, @richsalz ?)

@levitte
Copy link
Member

levitte commented Oct 11, 2017

BTW, does this also apply to 1.1.0?

@richsalz
Copy link
Contributor

Thanks, @levitte :)

@kaduk
Copy link
Contributor Author

kaduk commented Oct 11, 2017

The no-ec thing should not be needed on 1.1.0; the other one might be present there but I'm inclined to not worry about it until someone or something complains.

levitte pushed a commit that referenced this pull request Oct 11, 2017
Now that we are moving to support named FFDH groups, these fields are not
ec-specific, so we need them to always be available.

This fixes the no-ec --strict-warnings build, since gcc
5.4.0-6ubuntu1~16.04.4 appears to always try to compile the static inline
functions from ssl_locl.h, even when they are not used in the current
compilation unit.

Reviewed-by: Rich Salz <[email protected]>
Reviewed-by: Richard Levitte <[email protected]>
(Merged from #4518)
levitte pushed a commit that referenced this pull request Oct 11, 2017
test/bad_dtls_test.c: In function 'validate_client_hello':
test/bad_dtls_test.c:128:33: error: 'u' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     if (!PACKET_get_1(&pkt, &u) || u != SSL3_RT_HANDSHAKE)
                                 ^
Apparently -O1 does not perform sufficient optimization to ascertain
that PACKET_get_1 will always initialize u if it returns true.

Reviewed-by: Rich Salz <[email protected]>
Reviewed-by: Richard Levitte <[email protected]>
(Merged from #4518)
@richsalz richsalz added the approval: done This pull request has the required number of approvals label Oct 11, 2017
@kaduk
Copy link
Contributor Author

kaduk commented Oct 11, 2017

committed; closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: done This pull request has the required number of approvals

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants