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

X509_STORE_CTX accessors. #1044

Closed
wants to merge 3 commits into from
Closed

X509_STORE_CTX accessors. #1044

wants to merge 3 commits into from

Conversation

richsalz
Copy link
Contributor

@richsalz richsalz commented May 9, 2016

Add some functions that were missing when a number of X509
objects became opaque (thanks, Roumen!)

@richsalz richsalz added this to the 1.1.0 milestone May 10, 2016
@richsalz richsalz removed the 1.1 label May 10, 2016
@richsalz richsalz self-assigned this May 10, 2016
levitte
levitte reviewed May 12, 2016
View reviewed changes
crypto/x509/x509_lu.c
@@ -294,25 +294,22 @@ X509_LOOKUP *X509_STORE_add_lookup(X509_STORE *v, X509_LOOKUP_METHOD *m)
}
}

X509_OBJECT *X509_STORE_get_X509_by_subject(X509_STORE_CTX *vs, int type,
X509_NAME *name)
X509_OBJECT *X509_STORE_CTX_get_obj_by_subject(X509_STORE_CTX *vs, int type,
Copy link
Member

Choose a reason for hiding this comment

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

Please change the prefix from X509_STORE_CTX_ to X509_STORE_. You're not manipulating the ctx, nor are you retrieving values from it, you'r just using it to get information from the associated store, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but the arg is a store_ctx and so it's an operation on the STORE_CTX. the old name exists in the compatibility section.

Copy link
Member

Choose a reason for hiding this comment

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

(never mind)

@t-j-h
Copy link
Member

t-j-h commented May 12, 2016

If the functions take an X509_STORE_CTX as the first argument then they really should actually be named with X509_STORE_CTX as the prefix - so I guess @levitte and I have a different view on this one.

levitte
levitte reviewed May 12, 2016
View reviewed changes
crypto/x509/x509_lu.c
@@ -525,27 +526,28 @@ STACK_OF(X509_OBJECT) *X509_STORE_get0_objects(X509_STORE *v)
return v->objs;
}

STACK_OF(X509) *X509_STORE_get1_certs(X509_STORE_CTX *ctx, X509_NAME *nm)
STACK_OF(X509) *X509_STORE_CTX_get1_certs(X509_STORE_CTX *ctx, X509_NAME *nm)
Copy link
Member

Choose a reason for hiding this comment

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

Same thing here

Copy link
Contributor Author

@richsalz richsalz May 12, 2016 •
edited

Choose a reason for hiding this comment

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

same thing above. it's like having an RSA function take a PKEY parameter :)

Copy link
Member

Choose a reason for hiding this comment

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

(never mind)

@levitte
Copy link
Member

levitte commented May 12, 2016

I've love to hear the reasoning for having X509_STORE_CTX_ as a prefix just because you use a context. I compare this to the use of BN_CTX in the bignum unit, we have a large number of functions that are using such a context, but you don't see them having BN_CTX_ for a prefix... only those actually manipulating the context itself (allocator, deallocator, start and end) have that prefix.

@richsalz
Copy link
Contributor Author

I think BN API's that don't take a BN are misnamed :)

@richsalz
Copy link
Contributor Author

ping @levitte and @t-j-h . Roumen (original person who noticed the issue) gave it his approval.

X509_STORE_CTX accessors.
815acea 
Add some functions that were missing when a number of X509
objects became opaque (thanks, Roumen!)
@richsalz
Copy link
Contributor Author

And I could not find an BN_xxx API that took a BN_CTX as its first argument.

@levitte
Copy link
Member

levitte commented May 16, 2016

First argument? What does that have to do with anything? The difference as I see it is if the function's main task is to manipulate the context variable or merely use it.

@richsalz
Copy link
Contributor Author

The context is opaque. I have no idea what the function is doing :)

@richsalz richsalz closed this May 16, 2016
@richsalz richsalz reopened this May 16, 2016
@levitte
Copy link
Member

levitte commented May 16, 2016

For the record, DrH agrees with @richsalz .

@richsalz
Copy link
Contributor Author

okay, so ... plus-one anyone?

levitte
levitte reviewed May 17, 2016
View reviewed changes
crypto/x509/x509_lu.c
X509err(X509_F_X509_STORE_ADD_CRL, ERR_R_MALLOC_FAILURE);
return 0;
}
obj = X509_OBJECT_new();
Copy link
Member

Choose a reason for hiding this comment

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

Check for NULL

@levitte
Copy link
Member

levitte commented May 17, 2016

Last remarks, this is not entirely plusone-ready

@richsalz
Copy link
Contributor Author

pushed new commit to address your feedback, @levitte

levitte
levitte reviewed May 17, 2016
View reviewed changes
crypto/x509/x509_lu.c
if (obj == NULL) {
X509err(X509_F_X509_STORE_ADD_CERT, ERR_R_MALLOC_FAILURE);
obj = X509_OBJECT_new();
if (obj == NULL)
Copy link
Member

Choose a reason for hiding this comment

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

So, sometimes this functions adds an error code on error, and sometimes not? That's not consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The X509_STORE_new sets an error code.

Copy link
Member

Choose a reason for hiding this comment

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

Oh! Ok.

levitte
levitte reviewed May 17, 2016
View reviewed changes
crypto/x509/x509_lu.c
{
int i, idx, cnt;
STACK_OF(X509) *sk;
STACK_OF(X509) *sk = sk_X509_new_null();
Copy link
Member

Choose a reason for hiding this comment

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

You know, considering how it's used, I would initialise sk to NULL here...

Copy link
Member

Choose a reason for hiding this comment

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

... and do the allocation just before the for loop

Copy link
Member

Choose a reason for hiding this comment

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

Your call

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. pushing new commit.

Copy link
Member

Choose a reason for hiding this comment

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

Have you pushed yet? Can't see the change...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha! I did "push gitlab" not "github" :)

@levitte
Copy link
Member

levitte commented May 17, 2016

Awright, I'm satisfied.

@levitte
Copy link
Member

levitte commented May 17, 2016

I suggest waiting for Travis and Appveyor before merging

@levitte
Copy link
Member

levitte commented May 17, 2016

Travis seems happy

@richsalz
Copy link
Contributor Author

done.

@richsalz richsalz closed this May 17, 2016
@richsalz richsalz deleted the x509lu-adds branch June 7, 2016 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@richsalz richsalz

Labels
None yet
Projects
None yet
Milestone
1.1.0 - initial release
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@richsalz @t-j-h @levitte