httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christophe JAILLET <christophe.jail...@wanadoo.fr>
Subject Re: svn commit: r1598946 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_fdpass.c
Date Thu, 12 Jun 2014 20:30:34 GMT
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


Mime
View raw message