Re: minor auth code cleanup

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