Skip to content

Fix #72733: [RFC] Expose getaddrinfo C function, and supporting connect/bind#2078

Merged
php-pulls merged 2 commits into
php:masterfrom
bp1222:fix-72733
Sep 2, 2016
Merged

Fix #72733: [RFC] Expose getaddrinfo C function, and supporting connect/bind#2078
php-pulls merged 2 commits into
php:masterfrom
bp1222:fix-72733

Conversation

@bp1222

@bp1222 bp1222 commented Aug 12, 2016

Copy link
Copy Markdown
Contributor

Feature request was to expose getaddrinfo(). I accomplish this
by having an array of resources that are the addrinfo structures.
The resources can be used in new functions to connect/bind, and
one function to examine the contents of the resource.

[RFC] - https://wiki.php.net/rfc/socket_getaddrinfo

bp1222 added 2 commits August 12, 2016 11:29
Feature request was to expose getaddrinfo().  I accomplish this
by having an array of resources that are the addrinfo structures.
The resources can be used in new functions to connect/bind, and
one function to examine the contents of the resource.
@bp1222 bp1222 changed the title Fix #72733: Expose getaddrinfo C function, and supporting connect/bind Fix #72733: [RFC] Expose getaddrinfo C function, and supporting connect/bind Sep 1, 2016
@php-pulls php-pulls merged commit d59af68 into php:master Sep 2, 2016
Comment thread ext/sockets/sockets.c
hints.ai_protocol = Z_LVAL_P(hint);
} else if (strcmp(ZSTR_VAL(key), "ai_family") == 0) {
hints.ai_family = Z_LVAL_P(hint);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All of these should be using zval_get_long instead of Z_LVAL_P, otherwise you'll get crashes if other types are used. Additionally the key comparisons should preferably use zend_string_equals_literal to be pedantic about nul bytes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, since hints is an array, nothing is previously checking to make sure that they are in fact decent long values. I follow you on that. I assume the same with the move away from strcmp(), which I'll change

@bp1222

bp1222 commented Sep 2, 2016

Copy link
Copy Markdown
Contributor Author

@nikic - Since this branch has been merged, should I create a new PR with these fixes?

@nikic

nikic commented Sep 3, 2016

Copy link
Copy Markdown
Member

@bp1222 Yeah, that would be great

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants