getnameinfo extra len checking

[Sean Boudreau]

Hi,

Does anyone know the history of this check in getnameinfo_inet()? Is it
needed? This breaks, for example 'nmblookup \*' from samba. FreeBSD,
linux don't so this.

#ifdef BSD4_4
if (sa->sa_len != salen)
return EAI_FAIL;
#endif

Regards,

-seanb

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Christos Zoulas]

Post by Sean Boudreau
Hi,
Does anyone know the history of this check in getnameinfo_inet()? Is it
needed? This breaks, for example 'nmblookup \*' from samba. FreeBSD,
linux don't so this.
#ifdef BSD4_4
if (sa->sa_len != salen)
return EAI_FAIL;
#endif

What is sa_len in that case? I think it is 0. If so, then we can allow
0 through.

christos


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Thor Lancelot Simon]

Post by Christos Zoulas
What is sa_len in that case? I think it is 0. If so, then we can allow
0 through.

What's setting 0? Not setting sa_len is bogus.

[Christos Zoulas]

On Jun 17, 5:25pm, ***@panix.com (Thor Lancelot Simon) wrote:
-- Subject: Re: getnameinfo extra len checking

| On Thu, Jun 17, 2010 at 09:02:16PM +0000, Christos Zoulas wrote:
| >
| > What is sa_len in that case? I think it is 0. If so, then we can allow
| > 0 through.
|
| What's setting 0? Not setting sa_len is bogus.

Right, linux code that does not set sa_len...

christos

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[der Mouse]

Post by Christos Zoulas

Post by Thor Lancelot Simon
What's setting 0? Not setting sa_len is bogus.

Right, linux code that does not set sa_len...

Actually, as far as I can tell, Linux code _cannot_ set sa_len; I have
yet to find a Linux version that has it available to set.

Do our userland libraries really need Linux bug-compatability (yes, I
consider this a Linux bug) ifdefs? Seems to me that's the question.

/~\ The ASCII Mouse
\ / Ribbon Campaign
X Against HTML ***@rodents-montreal.org
/ \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Sean Boudreau]

Post by Christos Zoulas
-- Subject: Re: getnameinfo extra len checking
| >
| > What is sa_len in that case? I think it is 0. If so, then we can allow
| > 0 through.
|
| What's setting 0? Not setting sa_len is bogus.
Right, linux code that does not set sa_len...

sa_len was 0 in this case. Playing devils advocate...
Having to specify it twice could be considered redundant.
There's precedence other than linux for removing this
check: [free][open][dragonfly]bsd.


Regards,

-seanb

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Sean Boudreau]

Post by Christos Zoulas
-- Subject: Re: getnameinfo extra len checking
| >
| > What is sa_len in that case? I think it is 0. If so, then we can allow
| > 0 through.
|
| What's setting 0? Not setting sa_len is bogus.
Right, linux code that does not set sa_len...

So I'm leaning towards removing this check enirely. Would
that be acceptable?

Regards,

-seanb

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Christos Zoulas]

Post by Sean Boudreau

Post by Christos Zoulas
-- Subject: Re: getnameinfo extra len checking
| >
| > What is sa_len in that case? I think it is 0. If so, then we can allow
| > 0 through.
|
| What's setting 0? Not setting sa_len is bogus.
Right, linux code that does not set sa_len...

So I'm leaning towards removing this check enirely. Would
that be acceptable?

I think so. The kernel does not pay attention to sa_len either in syscalls
that take an extra salen argument (sys_bind() for example).

christos


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Thor Lancelot Simon]

Post by Sean Boudreau

Post by Christos Zoulas
-- Subject: Re: getnameinfo extra len checking
| >
| > What is sa_len in that case? I think it is 0. If so, then we can allow
| > 0 through.
|
| What's setting 0? Not setting sa_len is bogus.
Right, linux code that does not set sa_len...

So I'm leaning towards removing this check enirely. Would
that be acceptable?

I do not think so. Why should we pander to broken code that doesn't
set sa_len?

Thor

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Antti Kantee]

Post by Thor Lancelot Simon

Post by Sean Boudreau

Post by Christos Zoulas
-- Subject: Re: getnameinfo extra len checking
| >
| > What is sa_len in that case? I think it is 0. If so, then we can allow
| > 0 through.
|
| What's setting 0? Not setting sa_len is bogus.
Right, linux code that does not set sa_len...

So I'm leaning towards removing this check enirely. Would
that be acceptable?

I do not think so. Why should we pander to broken code that doesn't
set sa_len?

What is the purpose of sa_len? The value is a function of sa_family,
is it not? IIRC the only place where the value in sockaddr is really
needed is the radix code. The requirements of the radix code should
not be exposed to the whole world. *That* is broken if anything.

(apologies if my memory on the subject is too hazy to be useful)

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Sean Boudreau]

Post by Thor Lancelot Simon

Post by Sean Boudreau

Post by Christos Zoulas
-- Subject: Re: getnameinfo extra len checking
| >
| > What is sa_len in that case? I think it is 0. If so, then we can allow
| > 0 through.
|
| What's setting 0? Not setting sa_len is bogus.
Right, linux code that does not set sa_len...

So I'm leaning towards removing this check enirely. Would
that be acceptable?

I do not think so. Why should we pander to broken code that doesn't
set sa_len?

I guess it's a question of how pragmatic we want to be. I suspect
we'll see more code like this since the trend seems to be away
from this restriction.

Regards,

-seanb

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[der Mouse]

Post by Antti Kantee
What is the purpose of sa_len?

I don't know, since I didn't create it. But to me, it's nice to make
the data self-describing. I've recently had occasion to try to port
some of my code to Linux, which (as mentioned upthread) does not have
sa_len, and I find it is a significant inconvenience.

Post by Antti Kantee
The value is a function of sa_family, is it not?

Only partially. For some address families, it also depends on the
particular address. AF_LOCAL is an example; my memory is fuzzy, but I
*think* AF_DECnet would be another if we implemented it - IIRC their
analog of UDP/TCP port numbers is variable-length strings.

/~\ The ASCII Mouse
\ / Ribbon Campaign
X Against HTML ***@rodents-montreal.org
/ \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Antti Kantee]

Post by Antti Kantee

Post by Thor Lancelot Simon
I do not think so. Why should we pander to broken code that doesn't
set sa_len?

What is the purpose of sa_len? The value is a function of sa_family,
is it not?
From one point of view, yes. But remember, this is crappy object

"polymorphism" done in plain C. You get to maintain a pile of macros
that map every sa_family to its expected size and then violate the
opacity of the sockaddr to check the family, figure out the expected
size, and proceed accordingly. Ugh.
Or you can use sa_len as I assume
was expected, and treat opaque network addresses like opaque network
addresses: your program doesn't need to know what kind of address nor
how big that kind of address was intended to be; it's just "an address"
of size sa_len, and you don't poke at any other members of the
datastructure unless you actually _care_ what type it is. I find that
much cleaner.
Also, of course, checking sa_len against sa_family is an explicit
abstraction violation which is genuinely useful for detecting that
some part of your program is passing around corrupt sockaddrs. Don't
set sa_len, and you can't do that either.

Someone pointed out in private email it's used by our unix domain's
SUN_LEN(). I'm happy we don't have SIN_LEN() which subtracts the number
of zero bytes at the end of the IP address ;)

Aaaanyway, the whole issue is a little unfortunate given that SUSv3
doesn't require sa_len. Code which expects it to be present and set to
a non-garbage value is not portable (or callable from standard-conformant
code). So it's suckage either way ...

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[der Mouse]

I'm happy we don't have SIN_LEN() which subtracts the number of zero
bytes at the end of the IP address ;)

Heh. IMO those zero bytes are a horrible botch which should never have
been there and should not be checked by anything against anything.

Aaaanyway, the whole issue is a little unfortunate given that SUSv3
doesn't require sa_len.

I think this is a case where we should say "the standard is broken,
screw it, we're going to do it right". As Thor pointed out, you have
to either carry around a length field yourself with every sockaddr, you
have to do ugly layer-violating AF-aware hacks, or you have to use
sa_len. Like him, I agree that sa_len is the right way. (Though I
really think it should be wider than u_char; as it stands, there is no
way to set sa_len correctly if the address is longer than 255 bytes,
which AF_LOCAL may well be. Unfortunately that runs smack into ABI
combatability issues.)

/~\ The ASCII Mouse
\ / Ribbon Campaign
X Against HTML ***@rodents-montreal.org
/ \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Matthew Mondor]

On Mon, 21 Jun 2010 14:23:35 -0400 (EDT)

Post by der Mouse
really think it should be wider than u_char; as it stands, there is no
way to set sa_len correctly if the address is longer than 255 bytes,
which AF_LOCAL may well be. Unfortunately that runs smack into ABI
combatability issues.)

The sockaddr_un structure's sun_path field is defined with a fixed size
of 104 bytes, however. It's true that a path could exceed it, but
fortunately it's common practice to store AF_LOCAL socket files at
locations such as /var/run/ and /tmp/ which are short...

As for the getnameinfo(3) sa_len check, I was also bitten by it when
porting code over from Linux, and had to check the source to find out
the reason of the failure. However it was easy enough to fix that I
have no strong opinion about removing the sa_len check.

[Thor Lancelot Simon]

Post by Antti Kantee

Post by Thor Lancelot Simon
I do not think so. Why should we pander to broken code that doesn't
set sa_len?

What is the purpose of sa_len? The value is a function of sa_family,
is it not?

From one point of view, yes. But remember, this is crappy object
"polymorphism" done in plain C. You get to maintain a pile of macros
that map every sa_family to its expected size and then violate the
opacity of the sockaddr to check the family, figure out the expected
size, and proceed accordingly. Ugh.

Or you can use sa_len as I assume
was expected, and treat opaque network addresses like opaque network
addresses: your program doesn't need to know what kind of address nor
how big that kind of address was intended to be; it's just "an address"
of size sa_len, and you don't poke at any other members of the
datastructure unless you actually _care_ what type it is. I find that
much cleaner.

Also, of course, checking sa_len against sa_family is an explicit
abstraction violation which is genuinely useful for detecting that
some part of your program is passing around corrupt sockaddrs. Don't
set sa_len, and you can't do that either.

Thor

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Sean Boudreau]

Post by Sean Boudreau

Post by Christos Zoulas
-- Subject: Re: getnameinfo extra len checking
| >
| > What is sa_len in that case? I think it is 0. If so, then we can allow
| > 0 through.
|
| What's setting 0? Not setting sa_len is bogus.
Right, linux code that does not set sa_len...

So I'm leaning towards removing this check enirely. Would
that be acceptable?

Given that there's about an even split of opinion here
and that sa_len isn't checked anywhere else or passed
anywhere else that checks it, I've committed this.

Regards,

-seanb

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de