@martijn dot grendelman at isaac dot nl, thanks for reporting. Ref to bug #74216 for details. While it's a low security impact in fsockopen(), same piece is used for parsing in stream_socket_client().

Indeed, the "feature" is not documented so was not guaranteed to work. The way it is exploited is misleading, as tcp:// has no optional parameters. Nevertheless, probably we should indeed fix this case, especially the use case looks falling under the current scheme and the functionality itself makes sense. Fe one can say, a new optional parameter is introduced to tcp:// sheme, so then something like tcp://ip:port/id=bla would be a valid use. Or otherwise, for BC, anything after "/" would be read as an id. I'd suggest this

- don't affect the current fix to fsockopen
- add optional parameter to the tcp:// scheme in all branches, say id=bla
- otherwise treat anything before "/" as a unique id, if failed to parse

The last one - only for stable branches as a BC measure, master should be clean failing if the optional parameter couldn't be parsed, call it "id" or whatever. Sara, what do you think?

@martijn i also can't avoid mentioning, that this patch is around for more than a month and is also present in the RCs. So for one, it is sad to see pure QA participation quote, and it also probably shows the use case makes no wide impact.

Thanks.

Typo,

- otherwise treat anything AFTER "/" as a unique id, if failed to parse

Thanks.

Hi,

I'm just a user, and I normally don't run RCs. It takes time for PHP releases to make their way to the distributions and to users' computers, so I only noticed yesterday when upgrading one of our servers to PHP 7.0.18.

Cheers,
Martijn.

Here's another use case (one that I introduced, so I'm feeling guilty about): https://github.com/nrk/predis/pull/139

The problem being that Redis has the concept of multiple databases per server (so, the host, port etc. will match). So, if you want to use a persistent connections, you end up with cross-writes between the databases. (More worked example at that link.)

This user-contributed note should be amended/removed following an outcome: http://php.net/manual/en/function.stream-socket-client.php#105393

No. That "undocumented feature" was never intended to work, and the use of it is an abuse of the API.

As to allowing multiple persistent connections to a backend resource, I think we could add official support for it via a new API, and perhaps even matching this use case, but it's going to take some discussion because I don't want to trivially walk into codifying something for no better reason that "It worked on accident at some point."

For what it's worth, https://gist.github.com/sgolemon/51f329655a94523546f081bf9c9afd31 is probably safe enough to use as it would still cover the security case given by bug #74216, it would allow colinmollenhour's hack to keep working, and it's fairly "URI-ish", so it's not an unreasonable looking thing.

I'm not personally convinced though.

The fix for this bug has been committed.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.

 For Windows:

http://windows.php.net/snapshots/
 
Thank you for the report, and for helping us make PHP better.

Thanks for the patch ab! Further discussion of the future of this undocumented hack (as it has rightly been called), and any possible replacement (maybe a stream context option?) will, I presume, take place on the mailing list?

I'd recommend nuking the user note at http://php.net/manual/en/function.stream-socket-client.php#105393 to limit the spread of the problem, in any case, though.

I need to sit down later and collate all the myriad places bad transport strings get created (both intentionally and unintentionally) and I'll start a thread on internals to that effect.  I'll be happy to cc you at the time to make sure you see it. :D

Hello.

I'm the user who posted the user doc note (after accidently finding it when developing https://github.com/pear2/Net_Transmitter)

So... This is now "fixed" how? Is the after slash ID once again a thing for now, until a documented replacement is in place?

Abyway, as far as a replacement goes, I'd personally be perfectly fine with tcp://ip:port/id=bla AND breaking BC (since this was an undocumented feature/hack...).

Libraries like credis, predis and Net_Transmitter can easily be amended to prefix "id=" before the actual ID, which would work in all past versions where this undocumented feature/hack was working AND will work with that option introduced.

And BTW, if the PHP site allowed users to amend their user contributed doc notes, I totally would. Meanwhile, I hope you only nuke it after this feature gets properly introduced OR amend it to include a link to this bug report.

> So... This is now "fixed" how?

At the moment, we're reverted back to before bab0b99f so things are "fixed" in the sense that the status quo is maintained.  The named persistent sockets is a justifiable (if abusive) use of the socket streams API, so we're going to have to come up with a "right way" to do it.

I'm doubtful about being able to get that "right way" into 7.2 at this point (with feature freeze so close) so it'll probably end up in 7.3 which means we can't really remove the trailing identifiers support until some time after that.

BC is such a joy.

Does this mean #74192 needs to be reopened since the fix for it seems to be reverted?