I was attempting to get pg_autovacuum to work on my database and after much hammering at it I discovered that the stats system was not working.  I tried it with both 7.4beta1 and 7.4beta2 in both cases the number of tuples inserted, deleted and updated remained at 0 no matter what database activity occured.

Matthew T. O'Connor looked at my system recompiled postgres, and checked my config files and was unable to solve my problem.

I am using a default install of RedHat 9.0 on a VIA Samuel 2 processor.  Anyone have any ideas what could be causing my problem?  I have had hardware problems in the past but this is a new machine that I ran memtest86 and badblocks with the destructive tests on before I installed so I'm pretty sure thats not the issue.  Additionally I made a copy of the hard drive and placed it in another server and saw the same thing happen.

I did not have this problem with 7.3.3 so I think it has something to do with 7.4.

--- Adam Kavan--- akavan@cox.net

--- Adam Kavan
--- American Amuesments
--- akavan@cox.net
--- 402-499-5145


Wed, 3 Sep 2003 22:50:35 -0400
Adam Kavan <akavan@cox.net>
Stats Collector Error 7.4beta1 and 7.4beta2
Adam Kavan <akavan@cox.net> writes:
> I was attempting to get pg_autovacuum to work on my database and after
> much hammering at it I discovered that the stats system was not
> working.

Does 'ps' show that the stats collector and stats buffer postmaster
child processes are alive?  Are there any suggestive complaints in
the postmaster's log?

			regards, tom lane

Wed, 03 Sep 2003 23:50:27 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Stats Collector Error 7.4beta1 and 7.4beta2 
On Wed, 2003-09-03 at 23:50, Tom Lane wrote:
> Does 'ps' show that the stats collector and stats buffer postmaster
> child processes are alive?  Are there any suggestive complaints in
> the postmaster's log?

As Adam mentioned, I took a look at his system since the initial report
was about a problem with pg_autovacuum.  Anyway, Yes ps shows the two
stats collector related processes running, and no the log files don't
show anything helpful, however I didn't try to change any logging
settings.  Initially I saw an error in the logs about an IPv6 address
error but after I recompiled everthing with a simple ./configure
--prefix=/home/user/somethingelse/ I didn't get the IPv6 error in the
logs anymore.

Short answer is I have no idea why this is happening, and I didn't see
any obvious configuration problems that might cause this (make check
passed all tests).

Tom, Adam was able to give me a login to his machine, maybe he would do
the same for you.

Anyway, that is all I was able to see, hence the call for more help on
hackers :-)

Matthew



04 Sep 2003 01:10:55 -0400
"Matthew T. O'Connor" <matthew@zeut.net>
Re: Stats Collector Error 7.4beta1 and 7.4beta2
"Matthew T. O'Connor" <matthew@zeut.net> writes:
> ... Initially I saw an error in the logs about an IPv6 address
> error but after I recompiled everthing with a simple ./configure
> --prefix=/home/user/somethingelse/ I didn't get the IPv6 error in the
> logs anymore.

Hm.  Could it be an IPv6 issue --- that is, the stats collector is alive
and faithfully listening on some UDP port, but it's not the same port
the backends try to send to?  Given the discussion over the past couple
of days about bizarre interpretations of loopback addresses in
pg_hba.conf, I could sure believe there's some similar kind of issue for
the stats collector.

			regards, tom lane

Thu, 04 Sep 2003 01:23:28 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Stats Collector Error 7.4beta1 and 7.4beta2 
On Thu, 2003-09-04 at 01:23, Tom Lane wrote:
> Hm.  Could it be an IPv6 issue --- that is, the stats collector is alive
> and faithfully listening on some UDP port, but it's not the same port
> the backends try to send to?  Given the discussion over the past couple
> of days about bizarre interpretations of loopback addresses in
> pg_hba.conf, I could sure believe there's some similar kind of issue for
> the stats collector.

I had a similar thought, but I have no idea how I would verify this. 
The thing is, when I recompiled postgresql myself, I left pg_hba.conf at
default settings, and it's running on RH9, which I am running and have
not had a problem with...


04 Sep 2003 01:32:40 -0400
"Matthew T. O'Connor" <matthew@zeut.net>
Re: Stats Collector Error 7.4beta1 and 7.4beta2
Tom Lane wrote:
> "Matthew T. O'Connor" <matthew@zeut.net> writes:
> > ... Initially I saw an error in the logs about an IPv6 address
> > error but after I recompiled everthing with a simple ./configure
> > --prefix=/home/user/somethingelse/ I didn't get the IPv6 error in the
> > logs anymore.
> 
> Hm.  Could it be an IPv6 issue --- that is, the stats collector is alive
> and faithfully listening on some UDP port, but it's not the same port
> the backends try to send to?  Given the discussion over the past couple
> of days about bizarre interpretations of loopback addresses in
> pg_hba.conf, I could sure believe there's some similar kind of issue for
> the stats collector.

Doesn't the stats collector use unix domain sockets, not IP?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Thu, 4 Sep 2003 01:32:54 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Stats Collector Error 7.4beta1 and 7.4beta2
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Doesn't the stats collector use unix domain sockets, not IP?

No.  IIRC, we deliberately chose IP/UDP because it had buffering
behavior we liked.

There are pipes involved in the stats stuff too, but the weak link
in my mind is the backend-to-stats-buffer-process hop, which is UDP.

			regards, tom lane

Thu, 04 Sep 2003 01:39:04 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Stats Collector Error 7.4beta1 and 7.4beta2 
On Thu, 4 Sep 2003, Bruce Momjian wrote:

> Tom Lane wrote:
> > "Matthew T. O'Connor" <matthew@zeut.net> writes:
> > > ... Initially I saw an error in the logs about an IPv6 address
> > > error but after I recompiled everthing with a simple ./configure
> > > --prefix=/home/user/somethingelse/ I didn't get the IPv6 error in the
> > > logs anymore.
> > 
> > Hm.  Could it be an IPv6 issue --- that is, the stats collector is alive
> > and faithfully listening on some UDP port, but it's not the same port
> > the backends try to send to?  Given the discussion over the past couple
> > of days about bizarre interpretations of loopback addresses in
> > pg_hba.conf, I could sure believe there's some similar kind of issue for
> > the stats collector.
> 
> Doesn't the stats collector use unix domain sockets, not IP?

Nup.

    for (addr = addrs; addr; addr = addr->ai_next)
    {
#ifdef HAVE_UNIX_SOCKETS
        /* Ignore AF_UNIX sockets, if any are returned. */
        if (addr->ai_family == AF_UNIX)
            continue;
#endif
        if ((pgStatSock = socket(addr->ai_family, SOCK_DGRAM, 0)) >= 0)
            break;
    }

I thing I haven't seen asked: is there a packet filter blocking
local<->local UDP traffic by any chance?

Thanks,

Gavin


Thu, 4 Sep 2003 18:34:41 +1000 (EST)
Gavin Sherry <swm@linuxworld.com.au>
Re: Stats Collector Error 7.4beta1 and 7.4beta2

>
>I thing I haven't seen asked: is there a packet filter blocking
>local<->local UDP traffic by any chance?

Iptables is set to accept everything.  If it would help I can give you all 
log in information to poke around yourselves.  I appreciate your help.

--- Adam Kavan
--- akavan@cox.net 


Thu, 04 Sep 2003 11:21:48 -0500
Adam Kavan <akavan@cox.net>
Re: Stats Collector Error 7.4beta1 and 7.4beta2
On Thu, Sep 04, 2003 at 01:39:04AM -0400, Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Doesn't the stats collector use unix domain sockets, not IP?
> 
> No.  IIRC, we deliberately chose IP/UDP because it had buffering
> behavior we liked.

Once you said it was because not all platforms have unix domain
sockets.  I asked why we weren't using something like
socketpair().


Kurt


Thu, 4 Sep 2003 19:21:14 +0200
Kurt Roeckx <Q@ping.be>
Re: Stats Collector Error 7.4beta1 and 7.4beta2


Kurt Roeckx wrote:

> On Thu, Sep 04, 2003 at 01:39:04AM -0400, Tom Lane wrote:
>> Bruce Momjian <pgman@candle.pha.pa.us> writes:
>> > Doesn't the stats collector use unix domain sockets, not IP?
>> 
>> No.  IIRC, we deliberately chose IP/UDP because it had buffering
>> behavior we liked.
> 
> Once you said it was because not all platforms have unix domain
> sockets.  I asked why we weren't using something like
> socketpair().
> 

The reason to use INET UDP is that this is the only connection type that 
simply drops packets if the stupid collector daemon isn't able to keep 
up with the traffic. Think of a 64 processor SMP machine where 60 
backends utilize their own CPU and the poor little collector get's 
burried in packets, you don't want it to slow down the whole system, do you?

And I agree with Tom that it is very likely that the IPV4/IPV6 stuff is 
the reason. IIRC the postmaster creates the socket and noone ever does 
bind(2) on it - so it uses it's dynamically assigned port number. Both, 
the collector and the backends inherit that socket via fork(2). The 
backends use this socket with it's own sockname to send the stats out, 
and the collector reads it with recvfrom(2) and verifies that the from 
address is identical to it's sockname ... that way noone can inject 
faked stat packets. Now this is a lot of sockname usage that could lead 
to either the packets not arriving in the collector, or being thrown 
away by the collector because of failing to see them coming from itself.


Jan

> 
> Kurt
> 
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 2: you can get off all lists at once with the unregister command
>     (send "unregister YourEmailAddressHere" to majordomo@postgresql.org)

-- 
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me.                                  #
#================================================== JanWieck@Yahoo.com #


Thu, 04 Sep 2003 16:04:38 -0400
Jan Wieck <JanWieck@Yahoo.com>
Re: Stats Collector Error 7.4beta1 and 7.4beta2
On Thu, Sep 04, 2003 at 04:04:38PM -0400, Jan Wieck wrote:
> 
> And I agree with Tom that it is very likely that the IPV4/IPV6 stuff is 
> the reason. IIRC the postmaster creates the socket and noone ever does 
> bind(2) on it - so it uses it's dynamically assigned port number. Both, 
> the collector and the backends inherit that socket via fork(2).

Actually, it does a bind (to localhost), but send the port to 0,
so it gets the random port.

Then it connects to itself.  I don't get the logic behind that
howver.

It does:
pgStatSock = socket(...);
bind(pgStatSock, ...);
getsockname(pgStatSock, ...);
connect(pgStatSock, ...);

So it creates a socket, binds to it, asks what address/port it's
bound to, and connects to that port.

I don't see the logic behind that connect(), how it can work, and
how it would block anybody from sending to it, but it seems to
work.

> The 
> backends use this socket with it's own sockname to send the stats out, 
> and the collector reads it with recvfrom(2) and verifies that the from 
> address is identical to it's sockname ... that way noone can inject 
> faked stat packets. Now this is a lot of sockname usage that could lead 
> to either the packets not arriving in the collector, or being thrown 
> away by the collector because of failing to see them coming from itself.

I'm trying to think about some kernel bug that sends packets
using the wrong source address ..., but I think that was
connecting to a local address it always showed the loopback
address.

It could be useful to have a warning at the following line:

                        if (memcmp(&fromaddr, &pgStatAddr, fromlen))
                                continue;

That way you can rule out that that is a problem.

Anyway, I still didn't see the error message he got in the first
place.  Maybe we're looking at the wrong thing?


Kurt


Thu, 4 Sep 2003 22:37:38 +0200
Kurt Roeckx <Q@ping.be>
Re: Stats Collector Error 7.4beta1 and 7.4beta2


Kurt Roeckx wrote:


> It could be useful to have a warning at the following line:
> 
>                         if (memcmp(&fromaddr, &pgStatAddr, fromlen))
>                                 continue;
> 
> That way you can rule out that that is a problem.
> 
> Anyway, I still didn't see the error message he got in the first
> place.  Maybe we're looking at the wrong thing?

I think it's more this piece of code in postmaster/pgstat.c

         /*
          * The source address of the packet must be our own socket.
          * This ensures that only real hackers or our own backends
          * tell us something.  (This should be redundant with a
          * kernel-level check due to having used connect(), but let's
          * do it anyway.)
          */
         if (memcmp(&fromaddr, &pgStatAddr, fromlen))
             continue;


Jan

-- 
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me.                                  #
#================================================== JanWieck@Yahoo.com #


Thu, 04 Sep 2003 17:01:54 -0400
Jan Wieck <JanWieck@Yahoo.com>
Re: Stats Collector Error 7.4beta1 and 7.4beta2
On Thu, Sep 04, 2003 at 05:01:54PM -0400, Jan Wieck wrote:
> 
> 
> Kurt Roeckx wrote:
> 
> 
> >It could be useful to have a warning at the following line:
> >
> >                        if (memcmp(&fromaddr, &pgStatAddr, fromlen))
> >                                continue;
> >
> >That way you can rule out that that is a problem.
> >
> >Anyway, I still didn't see the error message he got in the first
> >place.  Maybe we're looking at the wrong thing?
> 
> I think it's more this piece of code in postmaster/pgstat.c

And what do you think I pasted?


Kurt


Thu, 4 Sep 2003 23:14:23 +0200
Kurt Roeckx <Q@ping.be>
Re: Stats Collector Error 7.4beta1 and 7.4beta2

>
>It could be useful to have a warning at the following line:
>
>                         if (memcmp(&fromaddr, &pgStatAddr, fromlen))
>                                 continue;
>
>That way you can rule out that that is a problem.
>
>Anyway, I still didn't see the error message he got in the first
>place.  Maybe we're looking at the wrong thing?
>
>
>Kurt

This is the very line that is giving me problems.  I commented it out and 
recompiled and now the stats system works.  Of course I have to assume that 
its bad to go around with out that check...

--- Adam Kavan
--- akavan@cox.net 


Thu, 04 Sep 2003 16:44:28 -0500
Adam Kavan <akavan@cox.net>
Re: Stats Collector Error 7.4beta1 and 7.4beta2
Adam Kavan <akavan@cox.net> writes:
>> if (memcmp(&fromaddr, &pgStatAddr, fromlen))
>> continue;

> This is the very line that is giving me problems.  I commented it out and 
> recompiled and now the stats system works.  Of course I have to assume that 
> its bad to go around with out that check...

Hmm.  Could you look and see what the actual values are in each address?

			regards, tom lane

Thu, 04 Sep 2003 18:49:18 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Stats Collector Error 7.4beta1 and 7.4beta2 
At 06:49 PM 9/4/03 -0400, Tom Lane wrote:
>Hmm.  Could you look and see what the actual values are in each address?
>
>                         regards, tom lane

I don't really know the layout of these structures so I dumped them to a 
file and attached them.  The first 16 bytes is from fromaddr and the second 
is from pgStatAddr.

--- Adam Kavan
--- akavan@cox.net
Thu, 04 Sep 2003 18:26:26 -0500
Adam Kavan <akavan@cox.net>
Re: Stats Collector Error 7.4beta1 and 7.4beta2 


Kurt Roeckx wrote:
> On Thu, Sep 04, 2003 at 05:01:54PM -0400, Jan Wieck wrote:
>> 
>> 
>> Kurt Roeckx wrote:
>> 
>> 
>> >It could be useful to have a warning at the following line:
>> >
>> >                        if (memcmp(&fromaddr, &pgStatAddr, fromlen))
>> >                                continue;
>> >
>> >That way you can rule out that that is a problem.
>> >
>> >Anyway, I still didn't see the error message he got in the first
>> >place.  Maybe we're looking at the wrong thing?
>> 
>> I think it's more this piece of code in postmaster/pgstat.c
> 
> And what do you think I pasted?

Hmmm ... good question ... How can I know what I think before I read 
what I write?


Jan :-)

> 
> 
> Kurt

-- 
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me.                                  #
#================================================== JanWieck@Yahoo.com #


Thu, 04 Sep 2003 20:03:13 -0400
Jan Wieck <JanWieck@Yahoo.com>
Re: Stats Collector Error 7.4beta1 and 7.4beta2
They are both structures of type sockaddr_in (sin_family 2 is AF_INET 
whereas sin_family 10 would've been AF_INET6), and all relevant fields 
of the structure look the same to me. The problem lies in the padding 
bytes that make sockaddr_in the same size as sockaddr.

Since the static structure pgStatAddr is supposed to be initialized to 
nul bytes by the compiler and now does not contain those in the padding 
area, my guess would be that getsockaddr() is actually writing garbage 
into that padding area. This is a nasty change, as one cannot compare 
two addresses for equalness with memcmp() any more just because of 
sloppy programming in the IP stack.

Well, the correct fix would be to compare only the relevant parts of the 
addresses, depending on the address family type.

I personally wouldn't worry too much about removing the check entirely. 
If you got a hacker wasting his time and bandwidth with screwing up your 
statistic collector daemon by sending faked UDP packets to some guessed 
port number (it's only visible in the netstat output on your local 
machine), I think he's done with all the rest of his TODO for the day 
and you'll soon face other problems than that.


Jan

Adam Kavan wrote:

> At 06:49 PM 9/4/03 -0400, Tom Lane wrote:
>>Hmm.  Could you look and see what the actual values are in each address?
>>
>>                         regards, tom lane
> 
> I don't really know the layout of these structures so I dumped them to a 
> file and attached them.  The first 16 bytes is from fromaddr and the second 
> is from pgStatAddr.
> 
> --- Adam Kavan
> --- akavan@cox.net 

-- 
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me.                                  #
#================================================== JanWieck@Yahoo.com #


Thu, 04 Sep 2003 20:36:21 -0400
Jan Wieck <JanWieck@Yahoo.com>
Re: Stats Collector Error 7.4beta1 and 7.4beta2
Adam Kavan <akavan@cox.net> writes:
> I don't really know the layout of these structures so I dumped them to a 
> file and attached them.  The first 16 bytes is from fromaddr and the second 
> is from pgStatAddr.

More legibly:

0000000 0200 8016 7f00 0001 0000 0000 0000 0000
0000010 0200 8016 7f00 0001 0000 0000 f001 0000

The 7f000001 is the IP loopback address, sure enough.  I wonder what the
f001 (or it might be little-endian 01f0) is.

			regards, tom lane

Thu, 04 Sep 2003 20:38:38 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Stats Collector Error 7.4beta1 and 7.4beta2 
Kurt Roeckx <Q@ping.be> writes:
> Then it connects to itself.  I don't get the logic behind that
> howver.

At least on HPUX, the connect(2) man page saith

     If the socket is of type SOCK_DGRAM, connect() specifies the peer
     address to which messages are to be sent, and the call returns
     immediately.  Furthermore, this socket can only receive messages sent
     from this address.

The "furthermore" is what we are after.

			regards, tom lane

Thu, 04 Sep 2003 20:40:52 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Stats Collector Error 7.4beta1 and 7.4beta2 
Redhat 7.1 says

        The file descriptor sockfd must refer to a socket.  If the
        socket is of type SOCK_DGRAM then the serv_addr address is
        the address to which datagrams are sent  by  default,  and
        the  only  address  from which datagrams are received.  If

Looks like the test is obsolete. Any objections to remove it? Do people 
agree that it's a bugfix that can go into 7.4?


Jan

Tom Lane wrote:

> Kurt Roeckx <Q@ping.be> writes:
>> Then it connects to itself.  I don't get the logic behind that
>> howver.
> 
> At least on HPUX, the connect(2) man page saith
> 
>      If the socket is of type SOCK_DGRAM, connect() specifies the peer
>      address to which messages are to be sent, and the call returns
>      immediately.  Furthermore, this socket can only receive messages sent
>      from this address.
> 
> The "furthermore" is what we are after.
> 
> 			regards, tom lane

-- 
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me.                                  #
#================================================== JanWieck@Yahoo.com #


Fri, 05 Sep 2003 09:35:11 -0400
Jan Wieck <JanWieck@Yahoo.com>
Re: Stats Collector Error 7.4beta1 and 7.4beta2
Jan Wieck wrote:
> Redhat 7.1 says
> 
>         The file descriptor sockfd must refer to a socket.  If the
>         socket is of type SOCK_DGRAM then the serv_addr address is
>         the address to which datagrams are sent  by  default,  and
>         the  only  address  from which datagrams are received.  If
> 
> Looks like the test is obsolete. Any objections to remove it? Do people 
> agree that it's a bugfix that can go into 7.4?

Oh, it definitely has to be in 7.4.  We can't ship it in its broken
state.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Fri, 5 Sep 2003 10:03:18 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Stats Collector Error 7.4beta1 and 7.4beta2
Jan Wieck <JanWieck@Yahoo.com> writes:
> Redhat 7.1 says
>         The file descriptor sockfd must refer to a socket.  If the
>         socket is of type SOCK_DGRAM then the serv_addr address is
>         the address to which datagrams are sent  by  default,  and
>         the  only  address  from which datagrams are received.  If

> Looks like the test is obsolete. Any objections to remove it? Do people 
> agree that it's a bugfix that can go into 7.4?

The test is not "obsolete"; we understood when we wrote it that it
should theoretically be redundant with the kernel-level restriction.
But there may be systems out there that fail to make the check promised
by the HPUX and Linux manpages.  So I'd prefer to leave it in there if
we can.  I don't want to rip it out simply because we don't understand
why it's failing on Adam's setup.

What I'm wondering about is whether we are comparing the right number of
bytes ... have both address structs been reported to have the same
length?  Maybe we need a min().

			regards, tom lane

Fri, 05 Sep 2003 11:23:20 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Stats Collector Error 7.4beta1 and 7.4beta2 


Tom Lane wrote:

> Jan Wieck <JanWieck@Yahoo.com> writes:
>> Redhat 7.1 says
>>         The file descriptor sockfd must refer to a socket.  If the
>>         socket is of type SOCK_DGRAM then the serv_addr address is
>>         the address to which datagrams are sent  by  default,  and
>>         the  only  address  from which datagrams are received.  If
> 
>> Looks like the test is obsolete. Any objections to remove it? Do people 
>> agree that it's a bugfix that can go into 7.4?
> 
> The test is not "obsolete"; we understood when we wrote it that it
> should theoretically be redundant with the kernel-level restriction.
> But there may be systems out there that fail to make the check promised
> by the HPUX and Linux manpages.  So I'd prefer to leave it in there if
> we can.  I don't want to rip it out simply because we don't understand
> why it's failing on Adam's setup.
> 
> What I'm wondering about is whether we are comparing the right number of
> bytes ... have both address structs been reported to have the same
> length?  Maybe we need a min().

I disagree. If getsockname(), getpeername() or recvfrom() return 
different address length's, it'd be more an indicator that the addresses 
ARE different anyway. Just because an IPV4 address doesn't have that 
feature is no reason to assume that addresses of different length are 
the same if they match over min() bytes.

If you want to continue to check the addresses and feel that HPUX and 
Linux manpage claims about kernel behaviour aren't enough, then we have 
to make it a specific check per "supported" AF. For now, that'd just be 
AF_INET and AF_INET6, and the default case bailing out with an error.


Jan

-- 
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me.                                  #
#================================================== JanWieck@Yahoo.com #


Fri, 05 Sep 2003 12:32:12 -0400
Jan Wieck <JanWieck@Yahoo.com>
Re: Stats Collector Error 7.4beta1 and 7.4beta2
On Fri, Sep 05, 2003 at 09:35:11AM -0400, Jan Wieck wrote:
> Redhat 7.1 says
> 
>        The file descriptor sockfd must refer to a socket.  If the
>        socket is of type SOCK_DGRAM then the serv_addr address is
>        the address to which datagrams are sent  by  default,  and
>        the  only  address  from which datagrams are received.  If
> 
> Looks like the test is obsolete. Any objections to remove it? Do people 
> agree that it's a bugfix that can go into 7.4?

Reading SUS v2 and v3, both say:

     If the initiating socket is not connection-mode, then connect()
     sets the socket's peer address, but no connection is made. For
     SOCK_DGRAM sockets, the peer address identifies where all datagrams
     are sent on subsequent send() calls, and limits the remote sender
     for subsequent recv() calls.


So it looks good to me.

I do wonder why nobody had a problem with this before however.


Kurt


Fri, 5 Sep 2003 19:01:37 +0200
Kurt Roeckx <Q@ping.be>
Re: Stats Collector Error 7.4beta1 and 7.4beta2
Jan Wieck <JanWieck@Yahoo.com> writes:
> Tom Lane wrote:
>> What I'm wondering about is whether we are comparing the right number of
>> bytes ... have both address structs been reported to have the same
>> length?  Maybe we need a min().

> I disagree. If getsockname(), getpeername() or recvfrom() return 
> different address length's, it'd be more an indicator that the addresses 
> ARE different anyway.

Hm, good point.  But I still feel that we are jumping to a conclusion
without understanding what's going on.  I'd like to know *why* the
addresses are different on Adam's machine, before we conclude that we
mustn't try to check that they are the same.

			regards, tom lane

Fri, 05 Sep 2003 13:02:07 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Stats Collector Error 7.4beta1 and 7.4beta2 
Tom Lane wrote:
> Jan Wieck <JanWieck@Yahoo.com> writes:
> > Tom Lane wrote:
> >> What I'm wondering about is whether we are comparing the right number of
> >> bytes ... have both address structs been reported to have the same
> >> length?  Maybe we need a min().
> 
> > I disagree. If getsockname(), getpeername() or recvfrom() return 
> > different address length's, it'd be more an indicator that the addresses 
> > ARE different anyway.
> 
> Hm, good point.  But I still feel that we are jumping to a conclusion
> without understanding what's going on.  I'd like to know *why* the
> addresses are different on Adam's machine, before we conclude that we
> mustn't try to check that they are the same.

Agreed.  We should know exactly what is happening.  My only point
earlier is that this has to be fixed for 7.4.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Fri, 5 Sep 2003 13:12:22 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Stats Collector Error 7.4beta1 and 7.4beta2
On a second thought,

I still believe that this is just garbage in the padding bytes after the 
IPV4 address. The code currently bind()'s and connect()'s explicitly to 
an AF_INET address. So all we ever should see is something from and 
AF_INET address. Everything else in the sin_family has to be discarded. 
I do not think it is allowed to bind() and connect() to an IPV4 address 
and then get anything other than an IPV4 address back from the system. 
If that is the case, the whole idea is broken.

An AF_INET address now has only two relevant fields, the sin_port and 
sin_addr. If they are the same, everything is fine. So the correct check 
would be that 1. fromlen > sizeof(sin_family), 2. sin_family == AF_INET, 
3. sin_port and sin_addr identical.

After reading Kurt's quoting of the SUS manpage I have to agree with Tom 
in that we cannot skip the check entirely. It says it limits for recv() 
but we are using recvfrom() ... there might be a little difference on 
that platform ...


Jan Wieck wrote:

> 
> Tom Lane wrote:
> 
>> Jan Wieck <JanWieck@Yahoo.com> writes:
>>> Redhat 7.1 says
>>>         The file descriptor sockfd must refer to a socket.  If the
>>>         socket is of type SOCK_DGRAM then the serv_addr address is
>>>         the address to which datagrams are sent  by  default,  and
>>>         the  only  address  from which datagrams are received.  If
>> 
>>> Looks like the test is obsolete. Any objections to remove it? Do people 
>>> agree that it's a bugfix that can go into 7.4?
>> 
>> The test is not "obsolete"; we understood when we wrote it that it
>> should theoretically be redundant with the kernel-level restriction.
>> But there may be systems out there that fail to make the check promised
>> by the HPUX and Linux manpages.  So I'd prefer to leave it in there if
>> we can.  I don't want to rip it out simply because we don't understand
>> why it's failing on Adam's setup.
>> 
>> What I'm wondering about is whether we are comparing the right number of
>> bytes ... have both address structs been reported to have the same
>> length?  Maybe we need a min().
> 
> I disagree. If getsockname(), getpeername() or recvfrom() return 
> different address length's, it'd be more an indicator that the addresses 
> ARE different anyway. Just because an IPV4 address doesn't have that 
> feature is no reason to assume that addresses of different length are 
> the same if they match over min() bytes.
> 
> If you want to continue to check the addresses and feel that HPUX and 
> Linux manpage claims about kernel behaviour aren't enough, then we have 
> to make it a specific check per "supported" AF. For now, that'd just be 
> AF_INET and AF_INET6, and the default case bailing out with an error.
> 
> 
> Jan
> 

-- 
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me.                                  #
#================================================== JanWieck@Yahoo.com #


Fri, 05 Sep 2003 14:04:26 -0400
Jan Wieck <JanWieck@Yahoo.com>
Re: Stats Collector Error 7.4beta1 and 7.4beta2
Jan Wieck <JanWieck@Yahoo.com> writes:
> I still believe that this is just garbage in the padding bytes after the 
> IPV4 address.

Yes.  I have been looking at the behavior on my own Linux box, on which
it turns out stats are broken too in CVS tip.  It is very clear that
getsockname() is returning garbage --- it says that the address length
is 16 bytes, but only the first 8 are actually used, and the values put
into the other 8 bytes vary from run to run.  recvfrom() also returns 16
bytes, but it seems to be careful to zero the extra space.  Ugh.

The reason 7.4 breaks where 7.3 worked is that 7.3 compared the
addresses like this:

            if (fromaddr.sin_addr.s_addr != pgStatAddr.sin_addr.s_addr)
                continue;
            if (fromaddr.sin_port != pgStatAddr.sin_port)
                continue;

where 7.4 has

            if (memcmp(&fromaddr, &pgStatAddr, fromlen))
                continue;

> The code currently bind()'s and connect()'s explicitly to 
> an AF_INET address.

Not in 7.4.  Kurt rewrote that stuff so it would still work on a machine
with only IPV6 addressing.  It should work for either AF_INET or
AF_INET6 sockets.  (I'm unconvinced that assuming "localhost" can be
looked up is an improvement over assuming "127.0.0.1" can be used, but
that's not our immediate problem.)

> After reading Kurt's quoting of the SUS manpage I have to agree with Tom 
> in that we cannot skip the check entirely. It says it limits for recv() 
> but we are using recvfrom() ... there might be a little difference on 
> that platform ...

I was about to say "I give up, let's just take out the comparison".
Your point is interesting but easily avoided; if we aren't going to check
fromaddr anymore then there's no need to use recvfrom(), it could as
well be recv() and save the kernel a few cycles.

			regards, tom lane

Fri, 05 Sep 2003 14:32:47 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Stats Collector Error 7.4beta1 and 7.4beta2 


Tom Lane wrote:

> I was about to say "I give up, let's just take out the comparison".
> Your point is interesting but easily avoided; if we aren't going to check
> fromaddr anymore then there's no need to use recvfrom(), it could as
> well be recv() and save the kernel a few cycles.

Which then get's us back to your concern about assuming that HPUX and 
Linux manpages can be taken as "every platform will" and hope all 
kernels will limit the sender for recv() to the connected address.

Since all involved processes are children of the postmaster, we can add 
some other, random number based security signature into the message 
itself. Noone outside will know what that is, it's really hard to guess 
and can be checked with a few int32 compares, not even a function call 
required.


Jan

-- 
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me.                                  #
#================================================== JanWieck@Yahoo.com #


Fri, 05 Sep 2003 14:50:11 -0400
Jan Wieck <JanWieck@Yahoo.com>
Re: Stats Collector Error 7.4beta1 and 7.4beta2
Jan Wieck <JanWieck@Yahoo.com> writes:
> Tom Lane wrote:
>> I was about to say "I give up, let's just take out the comparison".

> Which then get's us back to your concern about assuming that HPUX and 
> Linux manpages can be taken as "every platform will" and hope all 
> kernels will limit the sender for recv() to the connected address.

Well, I'd not have cared to trust just those couple of manpages, but
if it's in the Single Unix Spec then it's more likely that everyone
follows it.  Also, I checked my yellowing first edition of Stevens,
and it says the same thing: "only datagrams from this address will be
received by the socket".  So I'm thinking that this behavior has been
passed down from the original Berkeley sockets code.

> Since all involved processes are children of the postmaster, we can add 
> some other, random number based security signature into the message 
> itself. Noone outside will know what that is, it's really hard to guess 
> and can be checked with a few int32 compares, not even a function call 
> required.

We could do that if we're feeling paranoid, but I'm now leaning to the
view that it's not worth the trouble.

			regards, tom lane

Fri, 05 Sep 2003 14:59:36 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Stats Collector Error 7.4beta1 and 7.4beta2 

This analysis makes sense - I think using memcmp is clearly wrong here.

cheers

andrew

Jan Wieck said:
> On a second thought,
>
> I still believe that this is just garbage in the padding bytes after
> the  IPV4 address. The code currently bind()'s and connect()'s
> explicitly to  an AF_INET address. So all we ever should see is
> something from and  AF_INET address. Everything else in the sin_family
> has to be discarded.  I do not think it is allowed to bind() and
> connect() to an IPV4 address  and then get anything other than an IPV4
> address back from the system.  If that is the case, the whole idea is
> broken.
>
> An AF_INET address now has only two relevant fields, the sin_port and
> sin_addr. If they are the same, everything is fine. So the correct
> check  would be that 1. fromlen > sizeof(sin_family), 2. sin_family ==
> AF_INET,  3. sin_port and sin_addr identical.
>
> After reading Kurt's quoting of the SUS manpage I have to agree with
> Tom  in that we cannot skip the check entirely. It says it limits for
> recv()  but we are using recvfrom() ... there might be a little
> difference on  that platform ...
>
>




Fri, 5 Sep 2003 21:09:19 -0400 (EDT)
<andrew@dunslane.net>
Re: Stats Collector Error 7.4beta1 and 7.4beta2
<andrew@dunslane.net> writes:
> This analysis makes sense - I think using memcmp is clearly wrong here.

Yeah, now that I think about it, we're betting on the kernel to
faithfully zero all unused bits in addrinfo structures.  In an ideal
world, all kernels would do that, but in the real world it seems like
a losing bet.

I could go for Jan's idea of putting a random key into the messages,
if anyone feels that we should not trust to the kernel to enforce the
packet source address restriction.  But the memcmp() test seems a clear
loser given today's discussions.

			regards, tom lane

Fri, 05 Sep 2003 22:26:19 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Stats Collector Error 7.4beta1 and 7.4beta2 
Tom Lane wrote:
> <andrew@dunslane.net> writes:
> > This analysis makes sense - I think using memcmp is clearly wrong here.
> 
> Yeah, now that I think about it, we're betting on the kernel to
> faithfully zero all unused bits in addrinfo structures.  In an ideal
> world, all kernels would do that, but in the real world it seems like
> a losing bet.

Yeah, I've always been under the impression that it's a bad idea in
general to memcmp() structs, if only because in doing so you make a
lot of implicit assumptions about the structs in question that aren't
necessarily true, especially when dealing with multiple architectures.

Makes me wonder if there are other parts of the code where we're
vulnerable to the same sort of issue...

> I could go for Jan's idea of putting a random key into the messages,
> if anyone feels that we should not trust to the kernel to enforce the
> packet source address restriction.  But the memcmp() test seems a clear
> loser given today's discussions.

The test in the 7.3.x code looked reasonable to me, especially if it's
possible to make it work with IPV6 (if it doesn't already).  It's doing
basically the right thing, at any rate: directly comparing the actual
fields that are relevant.  Does this test represent a significant
performance hit?




-- 
Kevin Brown					      kevin@sysexperts.com

Tue, 9 Sep 2003 02:10:20 -0700
Kevin Brown <kevin@sysexperts.com>
Re: Stats Collector Error 7.4beta1 and 7.4beta2
On Tue, Sep 09, 2003 at 02:10:20AM -0700, Kevin Brown wrote:
> > I could go for Jan's idea of putting a random key into the messages,
> > if anyone feels that we should not trust to the kernel to enforce the
> > packet source address restriction.  But the memcmp() test seems a clear
> > loser given today's discussions.
> 
> The test in the 7.3.x code looked reasonable to me, especially if it's
> possible to make it work with IPV6 (if it doesn't already).  It's doing
> basically the right thing, at any rate: directly comparing the actual
> fields that are relevant.  Does this test represent a significant
> performance hit?

The reason I used a memcmp() instead of dealing with the
structure members themself is because it was easier.

Checking that they're the same address family is easy, and if
they're different the kernel is really broken.

For the addresses and port, in case of IPv4, you have to cast it
to sockaddr_in *, and compare the sin_addr and sin_port like
before.
For IPv6 you could do it with a memcmp on the sin6_addr part, and
put it inside an #ifdef HAVE_IPV6.

If you want to write code to compare 2 addresses, please make it
a general function and place it in ip.c.

Anyway, I'm happy with the current use of recv().


Kurt


Tue, 9 Sep 2003 20:03:41 +0200
Kurt Roeckx <Q@ping.be>
Re: Stats Collector Error 7.4beta1 and 7.4beta2


Kurt Roeckx wrote:

> On Tue, Sep 09, 2003 at 02:10:20AM -0700, Kevin Brown wrote:
>> > I could go for Jan's idea of putting a random key into the messages,
>> > if anyone feels that we should not trust to the kernel to enforce the
>> > packet source address restriction.  But the memcmp() test seems a clear
>> > loser given today's discussions.
>> 
>> The test in the 7.3.x code looked reasonable to me, especially if it's
>> possible to make it work with IPV6 (if it doesn't already).  It's doing
>> basically the right thing, at any rate: directly comparing the actual
>> fields that are relevant.  Does this test represent a significant
>> performance hit?
> 
> The reason I used a memcmp() instead of dealing with the
> structure members themself is because it was easier.

:-/

> 
> Checking that they're the same address family is easy, and if
> they're different the kernel is really broken.

Agreed.

> 
> For the addresses and port, in case of IPv4, you have to cast it
> to sockaddr_in *, and compare the sin_addr and sin_port like
> before.

Using a decent C compiler (and who compiles PostgreSQL without) this 
should reduce to some sort of a 32-bit and another 16-bit comparisions 
... no?

> For IPv6 you could do it with a memcmp on the sin6_addr part, and
> put it inside an #ifdef HAVE_IPV6.
> 
> If you want to write code to compare 2 addresses, please make it
> a general function and place it in ip.c.
> 
> Anyway, I'm happy with the current use of recv().

I disagree here. The clean and secure way is still to do *some* check 
(because we carefully don't assume that all OS's behave like some 
manpages happen to agree to). The performant way is to do it with 
information that is available without any extra system call.

So either we do the random signature thing, which I would favor as a one 
time be all, end all solution - or you do the actual from-address based 
implementation by restoring the old IPV4 behaviour and adding correct 
IPV6 behaviour.


Jan

-- 
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me.                                  #
#================================================== JanWieck@Yahoo.com #


Wed, 10 Sep 2003 01:50:34 -0400
Jan Wieck <JanWieck@Yahoo.com>
Re: Stats Collector Error 7.4beta1 and 7.4beta2
Jan Wieck <JanWieck@Yahoo.com> writes:
> So either we do the random signature thing, which I would favor as a one 
> time be all, end all solution - or you do the actual from-address based 
> implementation by restoring the old IPV4 behaviour and adding correct 
> IPV6 behaviour.

My feeling at this point is that it's not worth spending any effort on.
But if someone wants to expend effort, let's go with Jan's
random-signature idea.  That is simple, unquestionably portable, and
AFAICS it defends against more than the source-address check would
defend against, even after we got it right.  (Consider spoofed packet
source addresses.)

			regards, tom lane

Wed, 10 Sep 2003 02:01:09 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Stats Collector Error 7.4beta1 and 7.4beta2 
Tom Lane said:
> Jan Wieck <JanWieck@Yahoo.com> writes:
>> So either we do the random signature thing, which I would favor as a
>> one  time be all, end all solution - or you do the actual from-address
>> based  implementation by restoring the old IPV4 behaviour and adding
>> correct  IPV6 behaviour.
>
> My feeling at this point is that it's not worth spending any effort on.
> But if someone wants to expend effort, let's go with Jan's
> random-signature idea.  That is simple, unquestionably portable, and
> AFAICS it defends against more than the source-address check would
> defend against, even after we got it right.  (Consider spoofed packet
> source addresses.)
>

I see that currently the check has been removed rather than fixed.

If someone can spoof the packet address isn't there also a possibility
that they can read your packets and see your random signature?

I'm not clear what would be gained by an attacker being able to insert
such spoofed packets into the stream, though. IOW, how big is the security
threat?

cheers

andrew



Wed, 10 Sep 2003 07:27:02 -0400 (EDT)
"Andrew Dunstan" <andrew@dunslane.net>
Re: Stats Collector Error 7.4beta1 and 7.4beta2
On Wed, Sep 10, 2003 at 07:27:02AM -0400, Andrew Dunstan wrote:

> If someone can spoof the packet address isn't there also a possibility
> that they can read your packets and see your random signature?

Spoofing the packet source address is not quite the same as sniffing a
connection, which should be encrypted if you do not trust your
environment AFAIU.

> I'm not clear what would be gained by an attacker being able to insert
> such spoofed packets into the stream, though. IOW, how big is the security
> threat?

DoS caused by the autovacuum daemon running too much?

-- 
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"The Gord often wonders why people threaten never to come back after they've
been told never to return" (www.actsofgord.com)

Wed, 10 Sep 2003 09:55:38 -0400
Alvaro Herrera <alvherre@dcc.uchile.cl>
Re: Stats Collector Error 7.4beta1 and 7.4beta2
Jan Wieck wrote:
> > For IPv6 you could do it with a memcmp on the sin6_addr part, and
> > put it inside an #ifdef HAVE_IPV6.
> > 
> > If you want to write code to compare 2 addresses, please make it
> > a general function and place it in ip.c.
> > 
> > Anyway, I'm happy with the current use of recv().
> 
> I disagree here. The clean and secure way is still to do *some* check 
> (because we carefully don't assume that all OS's behave like some 
> manpages happen to agree to). The performant way is to do it with 

If the OS behavior doesn't match the man page, can't we just call that
an OS bug?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Wed, 10 Sep 2003 11:48:14 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Stats Collector Error 7.4beta1 and 7.4beta2
Alvaro Herrera <alvherre@dcc.uchile.cl> writes:
> On Wed, Sep 10, 2003 at 07:27:02AM -0400, Andrew Dunstan wrote:
>> If someone can spoof the packet address isn't there also a possibility
>> that they can read your packets and see your random signature?

> Spoofing the packet source address is not quite the same as sniffing a
> connection, which should be encrypted if you do not trust your
> environment AFAIU.

Remember this is a local-loopback connection; the packets will never
leave your own kernel.  If the attacker can sniff the packets then he is
already into your kernel, in which case game over.  But depending on how
careful your kernel is, it's possible that an attacker who doesn't yet
own your machine could inject forged packets with a local source
address.  So I think that indeed there are scenarios where a
random-signature check would be more secure than a source-address check.

The question is whether any of this is worth worrying about in PG.
ISTM the correct solution to such a risk is to tighten your kernel's
packet filtering, not harden one piece of one application.

			regards, tom lane

Wed, 10 Sep 2003 12:49:31 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Stats Collector Error 7.4beta1 and 7.4beta2 
On Wed, Sep 10, 2003 at 12:49:31 -0400,
  Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> The question is whether any of this is worth worrying about in PG.
> ISTM the correct solution to such a risk is to tighten your kernel's
> packet filtering, not harden one piece of one application.

On linux at least, it is pretty easy to make sure packets claiming to
be from loopback are dropped if they don't come in on the loopback
interface.

Wed, 10 Sep 2003 13:42:18 -0500
Bruno Wolff III <bruno@wolff.to>
Re: Stats Collector Error 7.4beta1 and 7.4beta2