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

const values e.g. const SSL* ssl #1526

Closed
wolftobias opened this issue Sep 1, 2016 · 12 comments
Closed

const values e.g. const SSL* ssl #1526

wolftobias opened this issue Sep 1, 2016 · 12 comments
Assignees

Comments

@wolftobias
Copy link

@wolftobias wolftobias commented Sep 1, 2016

We experienced a change between beta6 and release version 1.1.0 concerning const values.
At some stage you return a const SSL* which is fine, but at other places a non const parameter is required. That generates the need of unwanted casts.

parameter 1: const SSL* s
parameter 2: SSL_is_server((SSL)s)*

int security_callback(**const SSL* s,** const SSL_CTX* ctx, int op,
                                         int bits, int nid, void *other,
                                         void *ex) {
    int rc = 1;

    security_cb_ex *sdb = (security_cb_ex *)ex;

    switch (op) {
    case SSL_SECOP_TMP_DH: {
        DH* dh = NULL;
        if (**SSL_is_server((SSL*)s)** == 1) {
....
@richsalz

This comment has been minimized.

Copy link
Contributor

@richsalz richsalz commented Sep 1, 2016

That’s a bug, SSL_is_server() should take a const. Please list other places where you had to add casts and we’ll look at fixing them.

@wolftobias

This comment has been minimized.

Copy link
Author

@wolftobias wolftobias commented Sep 8, 2016

ASN1_TIME* t_not_before = (ASN1_TIME*)X509_get0_notBefore(x509);
ASN1_GENERALIZEDTIME* gt_not_before = ASN1_TIME_to_generalizedtime(t_not_before, NULL);
@wolftobias

This comment has been minimized.

Copy link
Author

@wolftobias wolftobias commented Sep 8, 2016

ASN1_TIME* t_not_after = (ASN1_TIME*)X509_get0_notAfter(x509);
ASN1_GENERALIZEDTIME* gt_not_after = ASN1_TIME_to_generalizedtime(t_not_after, NULL);
@richsalz

This comment has been minimized.

Copy link
Contributor

@richsalz richsalz commented Sep 8, 2016

In those examples, t_not_before and t_not_after should be const.

@levitte

This comment has been minimized.

Copy link
Member

@levitte levitte commented Jan 20, 2017

These were never fixed. We should have a second look.

@levitte levitte self-assigned this Jan 20, 2017
@richsalz

This comment has been minimized.

Copy link
Contributor

@richsalz richsalz commented Jan 20, 2017

Sadly, this is clearly an API change so 1.2 (which means 2018).

@levitte

This comment has been minimized.

Copy link
Member

@levitte levitte commented Jan 20, 2017

As before, I disagree.

The FAQ on version numbering as well as our release strategy talk about backward compatibility (both API and ABI). As far as I know, constifying function input parameters where appropriate does not constitute a backward incompatibility, it only explicitely asserts that we're not going to change the contents of the passed parameter.

This could at the very least be done in 1.1.1. (I'd argue that it's safe in 1.1.0 as well, but that's a fight I'm not as willing to take)

@richsalz

This comment has been minimized.

Copy link
Contributor

@richsalz richsalz commented Jan 20, 2017

I agree with you about parameters becoming const (although @dot-asm disagrees; we'll have to discuss and vote). But this case is return values

@levitte

This comment has been minimized.

Copy link
Member

@levitte levitte commented Jan 20, 2017

I think you've misunderstood. The way I read the complaint, it's that SSL_is_server takes a non-const SSL *, and that ASN1_TIME_to_generalizedtime takes a non-const ASN1_TIME. SSL_is_server clearly doesn't change the input, and I just checked, constifying the input ASN1_TIME parameter for ASN1_TIME_to_generalizedtime is without issues as well.

@richsalz

This comment has been minimized.

Copy link
Contributor

@richsalz richsalz commented Jan 20, 2017

you're right. oops.

mattcaswell added a commit to mattcaswell/openssl that referenced this issue May 2, 2017
mattcaswell added a commit to mattcaswell/openssl that referenced this issue May 2, 2017
@richsalz

This comment has been minimized.

Copy link
Contributor

@richsalz richsalz commented May 2, 2017

@mattcaswell

This comment has been minimized.

Copy link
Member

@mattcaswell mattcaswell commented May 2, 2017

This is #3360 which I opened earlier today.

levitte pushed a commit that referenced this issue May 19, 2017
Fixes #1526

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #3360)
(cherry picked from commit 6944311)
levitte pushed a commit that referenced this issue May 19, 2017
Fixes #1526

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #3360)
(cherry picked from commit 9bfeeef)
@levitte levitte closed this in 6944311 May 19, 2017
levitte pushed a commit that referenced this issue May 19, 2017
Fixes #1526

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #3360)
pracj3am added a commit to cdn77/openssl that referenced this issue Aug 22, 2017
Fixes openssl#1526

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from openssl#3360)
(cherry picked from commit 6944311)
pracj3am added a commit to cdn77/openssl that referenced this issue Aug 22, 2017
Fixes openssl#1526

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from openssl#3360)
(cherry picked from commit 9bfeeef)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.