Return-Path: X-Original-To: apmail-httpd-dev-archive@www.apache.org Delivered-To: apmail-httpd-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id A82711189C for ; Thu, 12 Jun 2014 20:31:06 +0000 (UTC) Received: (qmail 56968 invoked by uid 500); 12 Jun 2014 20:31:05 -0000 Delivered-To: apmail-httpd-dev-archive@httpd.apache.org Received: (qmail 56907 invoked by uid 500); 12 Jun 2014 20:31:05 -0000 Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm Precedence: bulk Reply-To: dev@httpd.apache.org list-help: list-unsubscribe: List-Post: List-Id: Delivered-To: mailing list dev@httpd.apache.org Received: (qmail 56899 invoked by uid 99); 12 Jun 2014 20:31:05 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 12 Jun 2014 20:31:05 +0000 X-ASF-Spam-Status: No, hits=2.2 required=5.0 tests=HTML_MESSAGE,RCVD_IN_DNSWL_NONE,SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (athena.apache.org: local policy) Received: from [80.12.242.128] (HELO smtp.smtpout.orange.fr) (80.12.242.128) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 12 Jun 2014 20:31:01 +0000 Received: from [192.168.1.12] ([92.140.230.67]) by mwinf5d12 with ME id DYWd1o00h1Ttze403YWePr; Thu, 12 Jun 2014 22:30:38 +0200 X-ME-Helo: [192.168.1.12] X-ME-Date: Thu, 12 Jun 2014 22:30:38 +0200 X-ME-IP: 92.140.230.67 Message-ID: <539A0DEA.9090302@wanadoo.fr> Date: Thu, 12 Jun 2014 22:30:34 +0200 From: Christophe JAILLET User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: dev@httpd.apache.org Subject: Re: svn commit: r1598946 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_fdpass.c References: <20140601065416.2BC112388980@eris.apache.org> In-Reply-To: Content-Type: multipart/alternative; boundary="------------090205030702040105040907" X-Virus-Checked: Checked by ClamAV on apache.org This is a multi-part message in MIME format. --------------090205030702040105040907 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Le 10/06/2014 18:19, Yann Ylavic a écrit : > On Sun, Jun 1, 2014 at 8:54 AM, wrote: >> Author: jailletc36 >> Date: Sun Jun 1 06:54:15 2014 >> New Revision: 1598946 >> >> URL: http://svn.apache.org/r1598946 >> Log: >> Fix computation of the size of 'struct sockaddr_un' when passed to 'connec()'. > s/connec()/connect()/ Thx Fixed in r1598946 > >> Use the same logic as the one in ' in 'proxy_util.c'. >> >> Modified: >> httpd/httpd/trunk/CHANGES >> httpd/httpd/trunk/modules/proxy/mod_proxy_fdpass.c >> > [...] >> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_fdpass.c >> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_fdpass.c?rev=1598946&r1=1598945&r2=1598946&view=diff >> ============================================================================== >> --- httpd/httpd/trunk/modules/proxy/mod_proxy_fdpass.c (original) >> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_fdpass.c Sun Jun 1 06:54:15 2014 >> @@ -55,6 +55,7 @@ static int proxy_fdpass_canon(request_re >> } >> >> /* TODO: In APR 2.x: Extend apr_sockaddr_t to possibly be a path !!! */ >> +/* XXX: The same function exists in proxy_util.c */ > Shouldn't there be ap_proxy_socket_connect_un() then? I didn't take time to do it but it was clearly what I had in mind with this note. Should I resubmit? > >> static apr_status_t socket_connect_un(apr_socket_t *sock, >> struct sockaddr_un *sa) >> { >> @@ -73,8 +74,9 @@ static apr_status_t socket_connect_un(ap >> } >> >> do { >> - rv = connect(rawsock, (struct sockaddr*)sa, >> - sizeof(*sa) + strlen(sa->sun_path)); >> + const socklen_t addrlen = APR_OFFSETOF(struct sockaddr_un, sun_path) >> + + strlen(sa->sun_path) + 1; > Do we need +1 here? > > It seems that cgid_init() and apr_generate_random_bytes() (when > HAVE_EGD) don't, whereas apr_sockaddr_info_get() and > apr_sockaddr_vars_set() use (the faulty) sizeof(struct sockaddr_un). > > Most examples I have found don't include the '\0' (so isn't the > SUN_LEN macro, which maybe you should use here). > According to man UNIX(7) though, the +1 seems to be included by > getsockname(), getpeername(), and accept() in their *addrlen output. > > Looks like connect() is not trusting the given length, and finds the > '\0' by itself... I didn't look at APR code and missed cgid with my grep. Just as you, I didn't manage to find some consistent information about it. - http://pubs.opengroup.org/onlinepubs/9699919799/functions/connect.html suggests that the size of the whole structure should be used (i.e./address_len:///Specifies the length of the *sockaddr* structure pointed to by the /address/ argument.) - most examples I found don't add the +1 for the last '\0' - some examples have this +1 (and this sound logical to me) - SUN_LEN does not seem to be that used One of the best post I've found is: http://stackoverflow.com/questions/2307511/proper-length-of-an-af-unix-socket-when-calling-bind In mod_proxy_fdpass and in proxy_util, this is safe because sun_path is filled with apr_cpystrn. So, leaving the +1 looks safe to me as we will go beyond sizeof(struct sockaddr_un). Any one else has an opinion? CJ --------------090205030702040105040907 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit
Le 10/06/2014 18:19, Yann Ylavic a écrit :
On Sun, Jun 1, 2014 at 8:54 AM,  <jailletc36@apache.org> wrote:

Author: jailletc36
Date: Sun Jun  1 06:54:15 2014
New Revision: 1598946

URL: http://svn.apache.org/r1598946
Log:
Fix computation of the size of 'struct sockaddr_un' when passed to 'connec()'.

s/connec()/connect()/
Thx Fixed in r1598946



Use the same logic as the one in ' in 'proxy_util.c'.

Modified:
    httpd/httpd/trunk/CHANGES
    httpd/httpd/trunk/modules/proxy/mod_proxy_fdpass.c


[...]

Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_fdpass.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_fdpass.c?rev=1598946&r1=1598945&r2=1598946&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/proxy/mod_proxy_fdpass.c (original)
+++ httpd/httpd/trunk/modules/proxy/mod_proxy_fdpass.c Sun Jun  1 06:54:15 2014
@@ -55,6 +55,7 @@ static int proxy_fdpass_canon(request_re
 }

 /* TODO: In APR 2.x: Extend apr_sockaddr_t to possibly be a path !!! */
+/* XXX: The same function exists in proxy_util.c */

Shouldn't there be ap_proxy_socket_connect_un() then?

I didn't take time to do it but it was clearly what I had in mind with this note.
Should I resubmit?


 static apr_status_t socket_connect_un(apr_socket_t *sock,
                                       struct sockaddr_un *sa)
 {
@@ -73,8 +74,9 @@ static apr_status_t socket_connect_un(ap
     }

     do {
-        rv = connect(rawsock, (struct sockaddr*)sa,
-                               sizeof(*sa) + strlen(sa->sun_path));
+        const socklen_t addrlen = APR_OFFSETOF(struct sockaddr_un, sun_path)
+                                  + strlen(sa->sun_path) + 1;

Do we need +1 here?

It seems that cgid_init() and apr_generate_random_bytes() (when
HAVE_EGD) don't, whereas apr_sockaddr_info_get() and
apr_sockaddr_vars_set() use (the faulty) sizeof(struct sockaddr_un).

Most examples I have found don't include the '\0' (so isn't the
SUN_LEN macro, which maybe you should use here).
According to man UNIX(7) though, the +1 seems to be included by
getsockname(), getpeername(), and accept() in their *addrlen output.

Looks like connect() is not trusting the given length, and finds the
'\0' by itself...
I didn't look at APR code and missed cgid with my grep.

Just as you, I didn't manage to find some consistent information about it.
    - http://pubs.opengroup.org/onlinepubs/9699919799/functions/connect.html suggests that the size of the whole structure should be used (i.e.address_len: Specifies the length of the sockaddr structure pointed to by the address argument.)
    - most examples I found don't add the +1 for the last '\0'
    - some examples have this +1 (and this sound logical to me)
    - SUN_LEN does not seem to be that used

One of the best post I've found is:
   http://stackoverflow.com/questions/2307511/proper-length-of-an-af-unix-socket-when-calling-bind

In mod_proxy_fdpass and in proxy_util, this is safe because sun_path is filled with apr_cpystrn.
So, leaving the +1 looks safe to me as we will go beyond sizeof(struct sockaddr_un).


Any one else has an opinion?

CJ

--------------090205030702040105040907--