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 |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Bruce Momjian | 2002-08-27 14:08:32 | Re: [Fwd: Re: libpgtcl modifications] |
Previous Message | Henshall, Stuart - WCP | 2002-08-27 11:58:37 | cygwin rename instead of link (7.2.2) |