Always create empty stream context to prevent race condition leading to CN mismatch on TLS enabled connections

[WyriHaximus]

Ran into an issue while connecting to both packagist and twitter at the same time. That resulted in the following error:

@DaveRandom jumped on that tweet and came with a repro and work around. This PR applies that workaround to the 0.4 branch, the master branch already has this fix in place. But I'll also test and confirm that to be sure.

[clue]

Thanks for spotting and filing a fix! 👍

I'm not sure if this should be considered a bug in PHP or just a documentation issue. Here's the simplest gist I could come up with that demonstrates this issue: https://3v4l.org/cdnV9

I think it makes sense to create an (empty) context for each connection so that setting context parameters does not affect other connections. 👍

Note that this particular error is the result of a higher level race condition if you concurrently establish multiple TLS connections. I'm okay with not having any tests for this because testing this will likely couple this to our particular implementation.

FWIW: This only affects v0.4.5 (not v0.5.0+ nor v0.4.4) because all other versions actually assign a default context.

[WyriHaximus]

Depending who you'd ask it is either a bug in PHP or the intended default behavior. Personally I consider this a bug.

Yeah honestly not 100% sure how to test against this. Unless we're going to include functional tests.

Glad to hear only v0.4.5 is affected, should a comment be added to this and the master branch regarding this? To ensure we don't accidentally remove it again

[DaveRandom]

PHP's behaviour is by design, but the design is wrong.

There will be a fix incoming soon, just as soon as we've figured out exactly what the scope of the fix should be and how/where it should be implemented, and possibly whether some elements may require an RFC - the default stream context has it's fingers in a lot of pies and there are other things internally that may potentially modify a context from a stream reference. Whatever the exact fix turns out to be and whenever it lands, this issue will still persist in historical versions though, and while those versions are supported this approach is still required (so at least until 7.1 is no longer supported, and it's unlikely to land before 7.2.0 at this point).

The long of the short of it is this: if you always explicitly pass in a stream context at the point of stream creation, you won't ever have a problem with this issue. Obviously there are also valid cases where having a shared stream context is valid (indeed it's currently unavoidable with peer sockets created by accept()), but this should still be done explicitly rather than using the magical global state of the default context.

I notice that your UnixConnector currently doesn't pass a context, it would possibly be worth doing it there as well, to head any potential weirdness off at the pass?

[DaveRandom]

As a side note, not everything will use a Connector for stream creation probably, so should probably put big fat warnings all over the docs about this issue as well.

[clue]

Updated documentation to include a warning about this in the master and v0.4 release branches.

Tagged and released as v0.4.6, as v0.4.5 is the only version affected by this race condition.

Thanks everybody for tracking this down! 👍