Skip site navigation (1) Skip section navigation (2)

Re: minor auth code cleanup

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Neil Conway <neilc(at)samurai(dot)com>
Cc: PostgreSQL Patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: minor auth code cleanup
Date: 2002-08-27 13:30:47
Message-ID: 25627.1030455047@sss.pgh.pa.us (view raw or flat)
Thread:
Lists: pgsql-patches
Neil Conway <neilc(at)samurai(dot)com> writes:
  
> ! 	/*
> ! 	 * We don't actually use the startup packet length the frontend sent
> ! 	 * us; however, it's a reasonable sanity check to ensure that we
> ! 	 * read as much data as we expected to.
> ! 	 *
> ! 	 * The actual startup packet size is the length of the buffer, plus
> ! 	 * the size part of the message (4 bytes), plus a terminator.
> ! 	 */
> ! 	Assert(len == (buf.len + 4 + 1));

This takes a non-problem and converts it into a problem, no?

There may be existing clients out there that miscompute the password
packet length.  Right now that does no harm.  With an Assert in place
in the backend, it will cause a database system restart.

Sir Mordred would be quite justified in labeling this a DOS
vulnerability...

On the pqcomm.h comment changes, I would like to see the options field
be variable-length too, with a fairly high upper limit since you might
want to fit several constructs like "-c guc_variable_name=value" in
there.  While at it we may as well get rid of the tty field, which is
unused since a long time.

On the subject of the timeout calculations, this code still looks
utterly bizarre:

> ! 			if (0 > (finish_time.tv_usec -= start_time.tv_usec))
> ! 			{
> ! 				remains.tv_sec++;
> ! 				finish_time.tv_usec += 1000000;
> ! 			}
> ! 			if (0 > (remains.tv_usec -= finish_time.tv_usec))
> ! 			{
> ! 				remains.tv_sec--;
> ! 				remains.tv_usec += 1000000;
> ! 			}
> ! 			remains.tv_sec -= finish_time.tv_sec - start_time.tv_sec;

It might be correct, I'm not sure; it's definitely going out of its way
to be confusing.  A more serious objection is that the code is actively
wrong on systems where tv_sec is unsigned, as for instance HPUX (dunno
whether that's standard or not).  If you manage to underflow
remains.tv_sec then you continue to wait forever ... or at least till
the long wraps around again...

			regards, tom lane

In response to

Responses

pgsql-patches by date

Next:From: Bruce MomjianDate: 2002-08-27 14:08:32
Subject: Re: [Fwd: Re: libpgtcl modifications]
Previous:From: Henshall, Stuart - WCPDate: 2002-08-27 11:58:37
Subject: cygwin rename instead of link (7.2.2)

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group