Date: Tue, 14 Jan 2020 15:12:07 -0500
From: Stephen Frost <sfrost@snowman.net>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Peter <pmc@citylink.dinoex.sub.org>, pgsql-hackers@lists.postgresql.org
Subject: Re: 12.1 not useable: clientlib fails after a dozen queries (GSSAPI
 ?)
Message-ID: <20200114201207.GA3195@tamriel.snowman.net>
References: <20200109181822.GA74698@gate.oper.dinoex.org>
 <7042.1578595681@sss.pgh.pa.us>
 <20200109205319.GA85142@gate.oper.dinoex.org>
 <15232.1578605504@sss.pgh.pa.us>
 <20200110022342.GA92515@gate.oper.dinoex.org>
 <12348.1578679149@sss.pgh.pa.us>
 <20200110210337.GA9580@gate.oper.dinoex.org>
 <26433.1578714710@sss.pgh.pa.us>
 <31477.1578756495@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="B68axRlCqbKl+xol"
Content-Disposition: inline
In-Reply-To: <31477.1578756495@sss.pgh.pa.us>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


--B68axRlCqbKl+xol
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> I wrote:
> > Here's a draft patch that cleans up all the logic errors I could find.
>=20
> So last night I was assuming that this problem just requires more careful
> attention to what to return in the error exit paths.  In the light of
> morning, though, I realize that the algorithms involved in
> be-secure-gssapi.c and fe-secure-gssapi.c are just fundamentally wrong:
>=20
> * On the read side, the code will keep looping until it gets a no-data
> error from the underlying socket call.  This is silly.  In every or
> almost every use, the caller's read length request corresponds to the
> size of a buffer that's meant to be larger than typical messages, so
> that betting that we're going to fill that buffer completely is the
> wrong way to bet.  Meanwhile, it's fairly likely that the incoming
> encrypted packet's length *does* correspond to some actual message
> boundary; that would only not happen if the sender is forced to break
> up a message, which ought to be a minority situation, else our buffer
> size choices are too small.  So it's very likely that the looping just
> results in doubling the number of read() calls that are made, with
> half of them failing with EWOULDBLOCK.  What we should do instead is
> return to the caller whenever we finish handing back the decrypted
> contents of a packet.  We can do the read() on the next call, after
> the caller's dealt with that data.

Yeah, I agree that this is a better approach.  Doing unnecessary
read()'s certainly isn't ideal but beyond being silly it doesn't sound
like this was fundamentally broken..? (yes, the error cases certainly
weren't properly being handled, I understand that)

> * On the write side, if the code encrypts some data and then gets
> EWOULDBLOCK trying to write it, it will tell the caller that it
> successfully wrote that data.  If that was all the data the caller
> had to write (again, not so unlikely) this is a catastrophic
> mistake, because the caller will be satisfied and will go to sleep,
> rather than calling again to attempt another write.  What we *must*
> do is to reflect the write failure verbatim whether or not we
> encrypted some data.  We must remember how much data we encrypted
> and then discount that much of the caller's supplied data next time.
> There are hints in the existing comments that somebody understood
> this at one point, but the code isn't acting that way today.

That's a case I hadn't considered and you're right- the algorithm
certainly wouldn't work in such a case.  I don't recall specifically if
the code had handled it better previously, or not, but I do recall there
was something previously about being given a buffer and then having the
API defined as "give me back the exact same buffer because I had to
stop" and I recall finding that to ugly, but I get it now, seeing this
issue.  I'd certainly be happier if there was a better alternative but I
don't know that there really is.

Thanks,

Stephen

--B68axRlCqbKl+xol
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJeHiCXAAoJEO1sijiDR2RVWzQP/3cNQCcrxQhduV1YuRateJ/n
sGqumrJyJuO+yaodhIIkZHAXsuRPwjIPrtO0XXdHTcwan57J7Qc4jfX7Hu2jvGmw
ua1dm8NQYxcBNrm03FO+Fw7rKPZ5yL7VgVLAJGMgIKEhdW8Wxw+VWpqvNZEm9CP1
MV14D6TH/CYJB9jEQEb0+KSKXXhvxPNa2A3rnrJ+w3xr51OENMwGPK0C4J3SkbmM
w8U3HmRx38Y3tYwNP7dJ0EMpVqsxBY4vgvBZER9KqRRUl8SOmwTWcXhi5ZxMYpxf
oc5pQxcB9n1lOPZGkXZ3xkK13oL6BX7gtCoYjMvqZc3AaTmL/YVf1jxilv0wCVa/
7H/R2Jk3vog5JSxIqKyhtTQSqMJaUL8bAldiqzPIobJJs8LdSpIaQZ+5T/l8izH3
+63Fi/alJcirpqKNHbVc7QEblhItBbrqPqwmJRFAQrg6jWv+yK2BKsd/ozdyKftS
Xp9SON3eeJDpSwmtcAJaM6vLfXPanRQxo3xztlD1IRu7i/NSyjrT8DdXlPoaVNGh
86mM/RwkprWx/CsqKwPFPLgQCend/yRM/y4LZq+2c+Bszq6z0xuZn8UtHYm08KN3
/nFB44v0ukvFTojpUSqME5gEdr/FU/EuPDwKZ3mXITcuhz7hv/BzCTj54bfWfSX3
gjnPBRa/Q7E9dIMaOIYk
=1SU2
-----END PGP SIGNATURE-----

--B68axRlCqbKl+xol--