Skip to content

Conversation

alex
Copy link
Member

@alex alex commented Aug 12, 2014

(Or any other field)

You can reproduce the error by running:

treq.get('https://nile.ghdonline.org')

from within a twisted program (and doing the approrpiate deferred stuff).

I'm unsure how to craft a unit test for this

(Or any other field)

You can reproduce the error by running:

```
treq.get('https://nile.ghdonline.org')
```

from within a twisted program (and doing the approrpiate deferred stuff).

I'm unsure how to craft a unit test for this
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) when pulling a268f6c on alex:aint-no-reason-things-are-this-way into 1110bc9 on pyca:master.

@exarkun
Copy link
Member

exarkun commented Aug 12, 2014

Thanks. Unfortunately, I'm not able to reproduce any error with that line.

@exarkun exarkun closed this Aug 12, 2014
@glyph
Copy link
Contributor

glyph commented Aug 12, 2014

I was able to reproduce it, but only with 0.9.8 / OS X. 1.0.1 / Ubuntu yielded no error. Perhaps OpenSSL has stopped providing NULL in the error queue in more recent versions?

@alex
Copy link
Member Author

alex commented Aug 12, 2014

Yeah, I can confirm that I don't see this with OpenSSL 1.01, only 0.9.8

@alex
Copy link
Member Author

alex commented Aug 12, 2014

It's not OS X specific though, it reproduces fine on Ubuntu Lucid.

@alex alex reopened this Aug 12, 2014
@glyph
Copy link
Contributor

glyph commented Aug 12, 2014

https://www.openssl.org/docs/crypto/err.html sort of implies, but doesn't outright state, that SSLerr(…, …); is a function or macro you could call. Might that be a way to unit test?

@glyph
Copy link
Contributor

glyph commented Aug 12, 2014

Scratch that, ERR_put_error is a real thing that you can actually use.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) when pulling a268f6c on alex:aint-no-reason-things-are-this-way into 1110bc9 on pyca:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling bae2dc3 on alex:aint-no-reason-things-are-this-way into 1110bc9 on pyca:master.

@alex
Copy link
Member Author

alex commented Aug 15, 2014

@glyph @exarkun there is now a test case using the strategy you outlined

Copy link
Contributor

Choose a reason for hiding this comment

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

Convention in pyOpenSSL seems to be to have sphinx docstrings on test methods, similar to Twisted style. Can you add one?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait… does not raise TypeError? Shouldn't it be talking about what it does raise?

ValueError is an exception which lots of built-in things raise and which might easily be confused if there's some other sort of implementation bug here; this is why I use ZeroDivisionError for my go-to "application exception" if I'm going to use a standard exception at all and not just define my own type.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling 27f55ed on alex:aint-no-reason-things-are-this-way into 1110bc9 on pyca:master.

@alex
Copy link
Member Author

alex commented Sep 17, 2014

Is there anything else needed here?

@sholsapp
Copy link
Contributor

Hi, sorry for chime in here so late, but I'm curious why an SSL error would ever not have a reason string? After reading through #135 more thoroughly, it sounds like we might be getting an X509_STORE_CTX context error here, which contains metadata about where the error is occurring... and has different OpenSSL API for fetching the error/reasons.

I'm mostly curious so I can watch out for the same class of errors in #155.

@alex
Copy link
Member Author

alex commented Sep 17, 2014

Sometimes OpenSSL, instead of using a reason which exists in their "reasons table" and therefore has a string, will just do some random arithmetic: https://github.com/openssl/openssl/blob/master/ssl/s23_clnt.c#L787

@sholsapp
Copy link
Contributor

Haha.. wow.. that's.. nice. In the case that you pointed out, do you know how to explain what is going on there (after superficial glance, this is illegible to me).

Are they truly doing some random arithmetic or are they including some context of the request in the error message (and thus doesn't have a pre-defined reason from their "reason table")?

@alex
Copy link
Member Author

alex commented Sep 17, 2014

If you know it's an error of that type, you can use the specific format to
extract some extra info, but I don't know if it's possible to do that if
you just know "it's a random error".

On Wed, Sep 17, 2014 at 2:18 PM, Stephen Holsapple <notifications@github.com

wrote:

Haha.. wow.. that's.. nice. In the case that you pointed out, do you know
how to explain what is going on there (after superficial glance, this is
illegible to me).

Are the truly doing some random arithmetic or are they including some
context of the request in the error message (and thus doesn't have a
pre-defined reason from their "reason table")?


Reply to this email directly or view it on GitHub
#147 (comment).

"I disapprove of what you say, but I will defend to the death your right to
say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero
GPG Key fingerprint: 125F 5C67 DFE9 4084

@sholsapp
Copy link
Contributor

Ah, I understand now, thank you. I'll try and see if similar issues exists in context errors.

@hynek
Copy link
Contributor

hynek commented Jan 6, 2015

This looks useful, correct and the builders agree. Anything more needed?

@alex
Copy link
Member Author

alex commented Jan 6, 2015

It feels RFC to me :-)

@glyph
Copy link
Contributor

glyph commented Jan 11, 2015

I'm inclined to agree with @hynek . Anything I can do to move this along?

@posita
Copy link

posita commented Apr 8, 2015

It feels RFC to me :-)

...

I'm inclined to agree with @hynek . Anything I can do to move this along?

Does this need a merge master first? It looks like trivial conflicts were introduced by de07546 et seq.

@exarkun exarkun merged commit 27f55ed into pyca:master Apr 11, 2015
@alex alex deleted the aint-no-reason-things-are-this-way branch April 11, 2015 11:37
@glyph
Copy link
Contributor

glyph commented Apr 14, 2015

Yay!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

7 participants