You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by "uhliarik (via GitHub)" <gi...@apache.org> on 2023/06/23 22:12:38 UTC
[GitHub] [apr] uhliarik opened a new pull request, #44: Fix socket creation when APR_UNSPEC is used
uhliarik opened a new pull request, #44:
URL: https://github.com/apache/apr/pull/44
When APR_UNSPEC is used, socket family is automaticaly set to APR_INET6 in apr_socket_create() and therefore if apr_sockaddr_info_get() finds out that the given address is in IPv4 format, it fails when calling apr_socket_connect(). So we first call apr_sockaddr_info_get() to get info about addr family and then pass it to apr_socket_create().
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@apr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apr] uhliarik commented on a diff in pull request #44: Fix socket creation when APR_UNSPEC is used
Posted by "uhliarik (via GitHub)" <gi...@apache.org>.
uhliarik commented on code in PR #44:
URL: https://github.com/apache/apr/pull/44#discussion_r1243494966
##########
memcache/apr_memcache.c:
##########
@@ -346,18 +346,37 @@ APR_DECLARE(apr_status_t) apr_memcache_disable_server(apr_memcache_t *mc, apr_me
return make_server_dead(mc, ms);
}
-static apr_status_t conn_connect(apr_memcache_conn_t *conn, apr_sockaddr_t *sa)
+static apr_status_t conn_connect(apr_memcache_conn_t *conn)
{
apr_status_t rv = APR_SUCCESS;
+ apr_sockaddr_t *sa;
+#if APR_HAVE_SOCKADDR_UN
+ apr_int32_t family = conn->ms->host[0] != '/' ? APR_UNSPEC : APR_UNIX;
+#else
+ apr_int32_t family = APR_UNSPEC;
+#endif
- rv = apr_socket_timeout_set(conn->sock, 1 * APR_USEC_PER_SEC);
+ rv = apr_sockaddr_info_get(&sa, conn->ms->host, family, conn->ms->port, 0, conn->p);
if (rv != APR_SUCCESS) {
return rv;
}
- rv = apr_socket_connect(conn->sock, sa);
- if (rv != APR_SUCCESS) {
- return rv;
+ /* Cycle through address until a connect() succeeds. */
+ for (; sa; sa = sa->next) {
+ rv = apr_socket_create(&conn->sock, sa->family, SOCK_STREAM, 0, conn->p);
+ if (rv == APR_SUCCESS) {
+ rv = apr_socket_timeout_set(conn->sock, 1 * APR_USEC_PER_SEC);
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
+
+ rv = apr_socket_connect(conn->sock, sa);
+ if (rv == APR_SUCCESS) {
+ break;
+ }
+
+ apr_socket_close(conn->sock);
+ }
}
Review Comment:
This would also work IMHO. I think, since we set apr_socket_timeout_set on closed socket, it would fail anyway... But it is better way how to handle it to explicitly say what we were not able to connect to any of given addresses.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@apr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apr] asfgit closed pull request #44: Fix socket creation when APR_UNSPEC is used
Posted by "asfgit (via GitHub)" <gi...@apache.org>.
asfgit closed pull request #44: Fix socket creation when APR_UNSPEC is used
URL: https://github.com/apache/apr/pull/44
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@apr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apr] notroj commented on a diff in pull request #44: Fix socket creation when APR_UNSPEC is used
Posted by "notroj (via GitHub)" <gi...@apache.org>.
notroj commented on code in PR #44:
URL: https://github.com/apache/apr/pull/44#discussion_r1243279746
##########
memcache/apr_memcache.c:
##########
@@ -346,18 +346,37 @@ APR_DECLARE(apr_status_t) apr_memcache_disable_server(apr_memcache_t *mc, apr_me
return make_server_dead(mc, ms);
}
-static apr_status_t conn_connect(apr_memcache_conn_t *conn, apr_sockaddr_t *sa)
+static apr_status_t conn_connect(apr_memcache_conn_t *conn)
{
apr_status_t rv = APR_SUCCESS;
+ apr_sockaddr_t *sa;
+#if APR_HAVE_SOCKADDR_UN
+ apr_int32_t family = conn->ms->host[0] != '/' ? APR_UNSPEC : APR_UNIX;
+#else
+ apr_int32_t family = APR_UNSPEC;
+#endif
- rv = apr_socket_timeout_set(conn->sock, 1 * APR_USEC_PER_SEC);
+ rv = apr_sockaddr_info_get(&sa, conn->ms->host, family, conn->ms->port, 0, conn->p);
if (rv != APR_SUCCESS) {
return rv;
}
- rv = apr_socket_connect(conn->sock, sa);
- if (rv != APR_SUCCESS) {
- return rv;
+ /* Cycle through address until a connect() succeeds. */
+ for (; sa; sa = sa->next) {
+ rv = apr_socket_create(&conn->sock, sa->family, SOCK_STREAM, 0, conn->p);
+ if (rv == APR_SUCCESS) {
+ rv = apr_socket_timeout_set(conn->sock, 1 * APR_USEC_PER_SEC);
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
+
+ rv = apr_socket_connect(conn->sock, sa);
+ if (rv == APR_SUCCESS) {
+ break;
+ }
+
+ apr_socket_close(conn->sock);
+ }
}
Review Comment:
This needs to catch the error case where no loop iteration succeeds, if sa is NULL after the for() loop then it should return with an error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@apr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apr] uhliarik commented on a diff in pull request #44: Fix socket creation when APR_UNSPEC is used
Posted by "uhliarik (via GitHub)" <gi...@apache.org>.
uhliarik commented on code in PR #44:
URL: https://github.com/apache/apr/pull/44#discussion_r1243494966
##########
memcache/apr_memcache.c:
##########
@@ -346,18 +346,37 @@ APR_DECLARE(apr_status_t) apr_memcache_disable_server(apr_memcache_t *mc, apr_me
return make_server_dead(mc, ms);
}
-static apr_status_t conn_connect(apr_memcache_conn_t *conn, apr_sockaddr_t *sa)
+static apr_status_t conn_connect(apr_memcache_conn_t *conn)
{
apr_status_t rv = APR_SUCCESS;
+ apr_sockaddr_t *sa;
+#if APR_HAVE_SOCKADDR_UN
+ apr_int32_t family = conn->ms->host[0] != '/' ? APR_UNSPEC : APR_UNIX;
+#else
+ apr_int32_t family = APR_UNSPEC;
+#endif
- rv = apr_socket_timeout_set(conn->sock, 1 * APR_USEC_PER_SEC);
+ rv = apr_sockaddr_info_get(&sa, conn->ms->host, family, conn->ms->port, 0, conn->p);
if (rv != APR_SUCCESS) {
return rv;
}
- rv = apr_socket_connect(conn->sock, sa);
- if (rv != APR_SUCCESS) {
- return rv;
+ /* Cycle through address until a connect() succeeds. */
+ for (; sa; sa = sa->next) {
+ rv = apr_socket_create(&conn->sock, sa->family, SOCK_STREAM, 0, conn->p);
+ if (rv == APR_SUCCESS) {
+ rv = apr_socket_timeout_set(conn->sock, 1 * APR_USEC_PER_SEC);
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
+
+ rv = apr_socket_connect(conn->sock, sa);
+ if (rv == APR_SUCCESS) {
+ break;
+ }
+
+ apr_socket_close(conn->sock);
+ }
}
Review Comment:
This would also work IMHO. I think, since we set apr_socket_timeout_set on closed socket, it would fail anyway... But it is the better way how to handle it - explicitly say that we were not able to connect to any of given addresses.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@apr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apr] uhliarik commented on pull request #44: Fix socket creation when APR_UNSPEC is used
Posted by "uhliarik (via GitHub)" <gi...@apache.org>.
uhliarik commented on PR #44:
URL: https://github.com/apache/apr/pull/44#issuecomment-1608549840
Altered the way how conn_connect() creates a socket connection. It should go now through all addresses returned by apr_sockaddr_info_get().
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@apr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apr] rpluem commented on a diff in pull request #44: Fix socket creation when APR_UNSPEC is used
Posted by "rpluem (via GitHub)" <gi...@apache.org>.
rpluem commented on code in PR #44:
URL: https://github.com/apache/apr/pull/44#discussion_r1243488779
##########
memcache/apr_memcache.c:
##########
@@ -346,18 +346,37 @@ APR_DECLARE(apr_status_t) apr_memcache_disable_server(apr_memcache_t *mc, apr_me
return make_server_dead(mc, ms);
}
-static apr_status_t conn_connect(apr_memcache_conn_t *conn, apr_sockaddr_t *sa)
+static apr_status_t conn_connect(apr_memcache_conn_t *conn)
{
apr_status_t rv = APR_SUCCESS;
+ apr_sockaddr_t *sa;
+#if APR_HAVE_SOCKADDR_UN
+ apr_int32_t family = conn->ms->host[0] != '/' ? APR_UNSPEC : APR_UNIX;
+#else
+ apr_int32_t family = APR_UNSPEC;
+#endif
- rv = apr_socket_timeout_set(conn->sock, 1 * APR_USEC_PER_SEC);
+ rv = apr_sockaddr_info_get(&sa, conn->ms->host, family, conn->ms->port, 0, conn->p);
if (rv != APR_SUCCESS) {
return rv;
}
- rv = apr_socket_connect(conn->sock, sa);
- if (rv != APR_SUCCESS) {
- return rv;
+ /* Cycle through address until a connect() succeeds. */
+ for (; sa; sa = sa->next) {
+ rv = apr_socket_create(&conn->sock, sa->family, SOCK_STREAM, 0, conn->p);
+ if (rv == APR_SUCCESS) {
+ rv = apr_socket_timeout_set(conn->sock, 1 * APR_USEC_PER_SEC);
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
+
+ rv = apr_socket_connect(conn->sock, sa);
+ if (rv == APR_SUCCESS) {
+ break;
+ }
+
+ apr_socket_close(conn->sock);
+ }
}
Review Comment:
Shouldn't the below directly after the for() loop fix this?
```suggestion
}
if (rv != APR_SUCCESS) {
return rv;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@apr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apr] notroj commented on pull request #44: Fix socket creation when APR_UNSPEC is used
Posted by "notroj (via GitHub)" <gi...@apache.org>.
notroj commented on PR #44:
URL: https://github.com/apache/apr/pull/44#issuecomment-1609581760
Thanks Lubos!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@apr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apr] uhliarik commented on pull request #44: Fix socket creation when APR_UNSPEC is used
Posted by "uhliarik (via GitHub)" <gi...@apache.org>.
uhliarik commented on PR #44:
URL: https://github.com/apache/apr/pull/44#issuecomment-1605047355
Follow up to https://svn.apache.org/repos/asf/apr/apr/trunk@1907242
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@apr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apr] uhliarik commented on pull request #44: Fix socket creation when APR_UNSPEC is used
Posted by "uhliarik (via GitHub)" <gi...@apache.org>.
uhliarik commented on PR #44:
URL: https://github.com/apache/apr/pull/44#issuecomment-1605050843
Reproducer:
run:
$ ./memcachecon 127.0.0.1 11211
$ ./memcachecon ::1 11211
```#include <apr.h>
#include <apr_memcache.h>
#include <stdlib.h>
int main(int argc, char **argv)
{
apr_pool_t *pglobal;
apr_status_t rv;
char *mcversion;
char *errstr = malloc(1024);
apr_app_initialize(NULL, NULL, NULL);
if (apr_pool_create(&pglobal, NULL)){
fprintf(stderr, "could not create global pool");
return 1;
}
if (argc != 3){
fprintf(stderr, "Usage: ./connconnectipv6 HOST PORT\n");
return 2;
}
apr_memcache_server_t *ms;
rv = apr_memcache_server_create(pglobal, argv[1], atoi(argv[2]), 0, 2, 2, 60, &ms);
if (rv != APR_SUCCESS){.
fprintf(stderr, "Errno: %d - %s\n", errno, apr_strerror(errno, errstr, 1024));
return 3;
}
rv = apr_memcache_version(ms, pglobal, &mcversion);
if (rv != APR_SUCCESS){.
fprintf(stderr, "Errno: %d - %s\n", errno, apr_strerror(errno, errstr, 1024));
return 5;
}
fprintf(stdout, "memcached version: %s\n", mcversion);
apr_pool_destroy(pglobal);
return 0;
}
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@apr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apr] uhliarik commented on a diff in pull request #44: Fix socket creation when APR_UNSPEC is used
Posted by "uhliarik (via GitHub)" <gi...@apache.org>.
uhliarik commented on code in PR #44:
URL: https://github.com/apache/apr/pull/44#discussion_r1243494966
##########
memcache/apr_memcache.c:
##########
@@ -346,18 +346,37 @@ APR_DECLARE(apr_status_t) apr_memcache_disable_server(apr_memcache_t *mc, apr_me
return make_server_dead(mc, ms);
}
-static apr_status_t conn_connect(apr_memcache_conn_t *conn, apr_sockaddr_t *sa)
+static apr_status_t conn_connect(apr_memcache_conn_t *conn)
{
apr_status_t rv = APR_SUCCESS;
+ apr_sockaddr_t *sa;
+#if APR_HAVE_SOCKADDR_UN
+ apr_int32_t family = conn->ms->host[0] != '/' ? APR_UNSPEC : APR_UNIX;
+#else
+ apr_int32_t family = APR_UNSPEC;
+#endif
- rv = apr_socket_timeout_set(conn->sock, 1 * APR_USEC_PER_SEC);
+ rv = apr_sockaddr_info_get(&sa, conn->ms->host, family, conn->ms->port, 0, conn->p);
if (rv != APR_SUCCESS) {
return rv;
}
- rv = apr_socket_connect(conn->sock, sa);
- if (rv != APR_SUCCESS) {
- return rv;
+ /* Cycle through address until a connect() succeeds. */
+ for (; sa; sa = sa->next) {
+ rv = apr_socket_create(&conn->sock, sa->family, SOCK_STREAM, 0, conn->p);
+ if (rv == APR_SUCCESS) {
+ rv = apr_socket_timeout_set(conn->sock, 1 * APR_USEC_PER_SEC);
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
+
+ rv = apr_socket_connect(conn->sock, sa);
+ if (rv == APR_SUCCESS) {
+ break;
+ }
+
+ apr_socket_close(conn->sock);
+ }
}
Review Comment:
This would also work IMHO. I think, since we set apr_socket_timeout_set on closed socket, it would fail anyway... But it is better way how to handle it - explicitly say that we were not able to connect to any of given addresses.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@apr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apr] uhliarik commented on pull request #44: Fix socket creation when APR_UNSPEC is used
Posted by "uhliarik (via GitHub)" <gi...@apache.org>.
uhliarik commented on PR #44:
URL: https://github.com/apache/apr/pull/44#issuecomment-1607292016
> Ah, hmm. I think this is still incorrect. The proper way is to loop over the returned addresses until a connect() succeeds. e.g. this:
>
> https://github.com/apache/httpd/blob/66254273e22a5b4051d15d7423b86a5d884b4ee5/modules/ssl/ssl_util_ocsp.c#L105
Ohh, I thought this job is done in apr_sockaddr_info_get --> find_addresses --> call_resolver():
https://github.com/apache/apr/blob/trunk/network_io/unix/sockaddr.c#L446
But this will only go through the list of addresses until the first AF_INET/AF_INET6 address is found. In case we pass hostname to it, it will indeed try only the first returned address which is not correct approach. I will to alter this patch and add the part where it will loop through all returned addresses until a connect() succeeds.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@apr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apr] notroj commented on pull request #44: Fix socket creation when APR_UNSPEC is used
Posted by "notroj (via GitHub)" <gi...@apache.org>.
notroj commented on PR #44:
URL: https://github.com/apache/apr/pull/44#issuecomment-1606965126
Ah, hmm. I think this is still incorrect. The proper way is to loop over the returned addresses until a connect() succeeds. e.g. this:
https://github.com/apache/httpd/blob/66254273e22a5b4051d15d7423b86a5d884b4ee5/modules/ssl/ssl_util_ocsp.c#L105
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@apr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org