On Thu, Jun 12, 2014 at 10:30 PM, Christophe JAILLET
<[email protected]> wrote:
> Le 10/06/2014 18:19, Yann Ylavic a écrit :
>
>> On Sun, Jun 1, 2014 at 8:54 AM, <[email protected]> wrote:
>>> /* 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?
I think you can add a new commit in the existing 2.4.x proposal since
there is no vote (other than yours) yet, and maybe requalify a bit the
message.
That will require a MMN minor bump though.
>>>
>>> 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).
The most important imho is to not truncate the length at sizeof(struct
sockaddr_un) when the real sun_path is beyond sizeof(sun_path).
The libc calls are probably bullet proof regarding NUL termination
(eg. force ((char*)sun)[addrlen] = 0 and recompute the length like in
the linux code from your link above), setting the NUL ourself at the
good place seems reasonable though ;)
I just find the NUL counted in the length quite disturbing, as it
looks like it is part of the path, and relies on libc's fixes (which
thus blatantly fails to handle given NULs in sun_paths :p )
Also, this can't be a general practice since it may make addrlen go
beyond sizeof(struct sockaddr_un) while it is not, and won't help (not
so common but safe) codes like :
f(const struct sockaddr_un *sun, socklen_t sunlen)
{
struct sockaddr_un buf, *copy = &buf;
if (sunlen < sizeof(buf) - 1)
buf = *sun;
else {
copy = malloc(sunlen + 1);
memcpy(copy, sun, sunlen);
}
((char*)copy)[sunlen] = 0;
/* ... play with non-const copy ... */
if (addrlen >= sizeof buf)
free(copy);
}
but this is certainly not what connect() does :)
Personnaly I would use SUN_LEN when defined or the version without the
+1 otherwise.
