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

Check for empty socket address #93

Closed
wants to merge 1 commit into from
Closed

Check for empty socket address #93

wants to merge 1 commit into from

Conversation

andig
Copy link
Contributor

@andig andig commented Apr 17, 2017

Fix #92

@clue
Copy link
Member

clue commented Apr 17, 2017

@andig Thanks for taking the time to file this PR! 👍

Unfortunately, the PR does not contain any documentation or tests, so it's a bit unclear what you're trying to achieve from the snippet you've posted.

May I ask you provide a simple gist of what you're trying to achieve, i.e. how can this be reproduced and how can we be sure this issue is actually resolved and we do not introduce any regressions in the future? Thanks!

@andig
Copy link
Contributor Author

andig commented Apr 17, 2017

May I ask you provide a simple gist of what you're trying to achieve, i.e. how can this be reproduced and how can we be sure this issue is actually resolved and we do not introduce any regressions in the future? Thanks!

This happens from inside our application. Unfortunately I cannot produce a simple gist. Looking at lxr I believe returning the NULL string is what PHP does- under which condition whatsoever.

I've added comment to the code what the check is supposed to do. There was no parseAddress test before and I have unfortunately no clue how to write one- sorry.

clue added a commit to clue-labs/socket that referenced this pull request Apr 22, 2017
@andig andig deleted the fix-92 branch May 8, 2017 15:06
@andig
Copy link
Contributor Author

andig commented May 8, 2017

Thanks for fixing. Now- for sake of documentation- I understand that the observed behaviour happens with UNIX domain sockets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants