Re: /etc/exports is now being read incorrectly

[Robert Elz]

Date:Wed, 23 Dec 2015 12:02:21 +0100
From:Martin Husemann 
Message-ID:  <20151223110221.ga6...@mail.duskware.de>

  | Can you (easily) test whether r1.113 of
  | src/lib/libc/net/getaddrinfo.c did this?

I haven't tested, but just from looking at it, it almost certainly did.

inet_aton() (and inet_pton()) for IPv4 typically treated dotted-quads
that weren't quads that way, and that commit made the code use inet_aton()
when it had not previously.

I'm not sure this is really worth changing though, expressing an IPv4
address literal in any form without all three dots has never been defined
to mean anything at all, and interpreting the address differently
depending upon the number of bits that end up being needed is just
a recipe for disaster.

kre

Re: /etc/exports is now being read incorrectly

[Robert Elz]

In my previous message, I meant to explicitly say that I'd just
change exports(5) to remove the couple of examples that use dotted-quads
with less than 3 dots.

If that's not considered approprite, then rather than reverting the
change to getaddrinfo(), change get_net() in usr.sin/moountd/moountd.c
to explicitly check for strings of digits and dots, and skip
calling getaddrinfo() in that case, judt doing the inet_network()
(and inet_makeaddr()) path instead.

It used to be that getaddrinfo() would tyically fail (one hoped) when
given a "name" like 1.2.3 (that is, it was hoped we wold not find that
in the DNS, and it failed to convert as a dotted quad, because of
insufficient fields).   The code in mountd then falls through and uses
inet_network() which (being ancient) accepts all the old crappy formats.

Now getaddrinfo() is no longer failing, but is (as it should, if they should
be interpreted at all) treating the dotted-"less than quad" in the old form
for an IP address (where 10.2 meant 10.0.0.2), which isn't what mountd
was expecting at all (it assumes getaddrinfo() will fail in such a case).

I'm not 100% sure what the getaddrinfo() is doing there at all ... it only
gets called when the first char of the "name" is a digit (what's special
about that case??) so most likely it is not really anticipating being called
with a host name, or FQDN, but only with an IP address.   Oh, I see  this
is the way IPv6 addresse literals are being handled .. if they happen to
start with a digit (2001:... but not c001:...) - ones that start with a
hex digit that is not a digit are handled later.

This whole section of code (get_net() in mountd) needs consigning to
/dev/null and being replaced by something sane.

kre

Re: /etc/exports is now being read incorrectly

[Christos Zoulas]

In article <4510.1450878...@andromeda.noi.kre.to>,
Robert Elz   wrote:
>In my previous message, I meant to explicitly say that I'd just
>change exports(5) to remove the couple of examples that use dotted-quads
>with less than 3 dots.
>
>If that's not considered approprite, then rather than reverting the
>change to getaddrinfo(), change get_net() in usr.sin/moountd/moountd.c
>to explicitly check for strings of digits and dots, and skip
>calling getaddrinfo() in that case, judt doing the inet_network()
>(and inet_makeaddr()) path instead.
>
>It used to be that getaddrinfo() would tyically fail (one hoped) when
>given a "name" like 1.2.3 (that is, it was hoped we wold not find that
>in the DNS, and it failed to convert as a dotted quad, because of
>insufficient fields).   The code in mountd then falls through and uses
>inet_network() which (being ancient) accepts all the old crappy formats.

Unfortunately the standards say it should parse that ol'style.

>Now getaddrinfo() is no longer failing, but is (as it should, if they should
>be interpreted at all) treating the dotted-"less than quad" in the old form
>for an IP address (where 10.2 meant 10.0.0.2), which isn't what mountd
>was expecting at all (it assumes getaddrinfo() will fail in such a case).
>
>I'm not 100% sure what the getaddrinfo() is doing there at all ... it only
>gets called when the first char of the "name" is a digit (what's special
>about that case??) so most likely it is not really anticipating being called
>with a host name, or FQDN, but only with an IP address.   Oh, I see  this
>is the way IPv6 addresse literals are being handled .. if they happen to
>start with a digit (2001:... but not c001:...) - ones that start with a
>hex digit that is not a digit are handled later.
>
>This whole section of code (get_net() in mountd) needs consigning to
>/dev/null and being replaced by something sane.

Heh, I put it in solitary confinement for now.

christos

Re: /etc/exports is now being read incorrectly

[Christos Zoulas]

In article <19881.1450911...@andromeda.noi.kre.to>,
Robert Elz   wrote:
>Date:Wed, 23 Dec 2015 16:22:05 + (UTC)
>From:chris...@astron.com (Christos Zoulas)
>Message-ID:  
>
>
>Looks like it has been slightly reformed by the experience too...

Still needs isolation and contemplation.

>While looking there I noticed a couple of nits (that have been there
>"forever" - nothing to do with your changes) but that you might want to
>fix while cleaning up...

[]

>What the comment should probably say is something like
>
>   /*
>* Disallow v6 addresses without a specific mask or masklen
>*/
>

Fixed.

>Also, even more minor (and again, old) there are space/tab mixups in the
>#defines that are now in mountd.h

Fixed.

>
>Lastly, I can fathom no reason at all for all the code calling getnameinfo()
>and setting nt_name - as best I can tell, nothing uses it.   It happens only
>in a subset of cases, so it is hard to believe that anything depends upon
>it that I have missed.   I think I'd just rip all of that nonsense out, and
>see what happens...

Doesn't put_exlist use it?

>The other call to getnameinfo() appears to be just to validate that the
>address just lovingly extracted from the string is in fact a valid addr.
>I'm not sure under what conditions that could fail (what binary addr form
>is going to be invald?   (In the sense of getnameinfo(AI_NUMERICHOST)
>failing anyway?)   Maybe for some other address family than v4 or v6, but
>this code is never going to work for them anyway.   So I think I'd rip
>out that one as well.

Again put_exlist uses that... But for what? We need to revisit the
export mess anyway. I just got bitten by wedge renumbering making my
fs exported invalid (and crashing ffs) since we still use dev_t for
fsid and that's not stable in a wedge world... There is also an
ancient PR about export issues.

christos

Re: /etc/exports is now being read incorrectly

[Robert Elz]

Date:Wed, 23 Dec 2015 16:22:05 + (UTC)
From:chris...@astron.com (Christos Zoulas)
Message-ID:  


  | Unfortunately the standards say it should parse that ol'style.

We don't necessarily have to blindly follow standards, we have to agree
that they are sane, and required, given actual current usage.

But in any case, I was not suggesting reverting the change to getaddrinfo().

  | Heh, I put it in solitary confinement for now.

Looks like it has been slightly reformed by the experience too...

While looking there I noticed a couple of nits (that have been there
"forever" - nothing to do with your changes) but that you might want to
fix while cleaning up...

First, the comment (now in get_net.c)

/*  
 * Only allow /pref notation for v6 addresses.
 */

is total nonsense.   What the code that follows actually does, is
require "/pref" (where "pref" as a name comes from for this purpose
is a whole other issue...) notation (except in the unlikely case that
the -mask option was already processed) for IPv6 addresses.

Aside from the order requirement for IPv6, which doesn't really matter,
as no-one sane would use -mask for IPv6 anyway, this is sensible, as
IPv6 has no "natural" mask length (unless, perhaps, one wanted to
default to /64, and that would be a bad idea) unlike the old address class
stuff from v4 that we have to continue supporting as there must be someone, 
somewhere in the world, who still uses it...

What the comment should probably say is something like

/*
 * Disallow v6 addresses without a specific mask or masklen
 */

Also, even more minor (and again, old) there are space/tab mixups in the
#defines that are now in mountd.h

Lastly, I can fathom no reason at all for all the code calling getnameinfo()
and setting nt_name - as best I can tell, nothing uses it.   It happens only
in a subset of cases, so it is hard to believe that anything depends upon
it that I have missed.   I think I'd just rip all of that nonsense out, and
see what happens...

The other call to getnameinfo() appears to be just to validate that the
address just lovingly extracted from the string is in fact a valid addr.
I'm not sure under what conditions that could fail (what binary addr form
is going to be invald?   (In the sense of getnameinfo(AI_NUMERICHOST)
failing anyway?)   Maybe for some other address family than v4 or v6, but
this code is never going to work for them anyway.   So I think I'd rip
out that one as well.

kre

/etc/exports is now being read incorrectly

[Geoff Wing]

Hi,
for a long time, the parser reading /etc/exports would treat the following
example from exports(5):

/u -maproot=bin: -network 131.104.48 -mask 255.255.255.0

as 131.104.48/24

Currently it's being treated as 131.104.0.48 (seen via ``showmount -e'')

I'm guessing it changed in the last month or so.

Regards,
Geoff