From: Jacob Champion <pchampion@vmware.com>
To: Daniel Gustafsson <daniel@yesql.se>
CC: Heikki Linnakangas <hlinnaka@iki.fi>, Andres Freund <andres@anarazel.de>,
	Postgres hackers <pgsql-hackers@lists.postgresql.org>, Michael Paquier
	<michael@paquier.xyz>, Andrew Dunstan <andrew.dunstan@2ndquadrant.com>,
	Stephen Frost <sfrost@snowman.net>, Thomas Munro <thomas.munro@gmail.com>
Subject: Re: Support for NSS as a libpq TLS backend
Thread-Topic: Support for NSS as a libpq TLS backend
Thread-Index: 
 AQHWM1w61GrKUIsl6UCBcYrYNRBt46kJRsIAgAA+PQCAGQsiAIAEdGUAgAAqk4CAAbpsAIABgI0AgCnqVACAA5UBgIAAY+MAgBTX7ACAACGAAIASngCAgAAfeICABYLFgIAbyjgAgABy24CACw63gIAAAzWAgAwa5QCAA6HPgIAAKuGAgAYXDoCAACZBAIABTCSAgAALLgCAAcj/gA==
Date: Thu, 12 Nov 2020 22:12:42 +0000
Message-ID: <FBA0C365-3B19-4E40-98F4-8B0064DB7127@vmware.com>
References: <e3c34e7b-900e-7e32-f60b-a278a1eb743a@2ndQuadrant.com>
 <DA91E5F0-5F9D-41A7-A7A6-B91CDE0F1D63@yesql.se>
 <CAA8=A7-suZg90M7Fgyo1Rfymkrpdh_Z547RKXrbHirsAmojQSw@mail.gmail.com>
 <20200904012334.GF19499@paquier.xyz> <20200917074134.GX2873@paquier.xyz>
 <BAD11CCA-E712-4906-8A27-94C2AA0E696E@yesql.se>
 <20200929055939.GF7117@paquier.xyz>
 <DE377D9A-BFFC-492C-A2B8-2AE6EBBBE740@yesql.se>
 <411593A7-E037-474D-BFD7-D3D6683C1D46@yesql.se>
 <B1ADEE76-3E33-4E9D-B830-3AA89B1C42D2@yesql.se>
 <20201020191529.5yverw3ybaube3pg@alap3.anarazel.de>
 <907FA7D1-9BD9-464B-A6EC-DEEB53438D36@yesql.se>
 <7ed636a0-872c-3ea7-dc44-b35dd515bb87@iki.fi>
 <02260088-E0C9-49A2-ACD3-CE51D65A84E3@yesql.se>
 <0E64F3CB-4731-4FA8-82D8-D8590086654C@vmware.com>
 <F16F003C-EAFB-4FCA-9880-3D5E2B428F78@yesql.se>
 <1B0B31E4-AD37-475C-9374-7E24AA808479@vmware.com>
 <B506D54C-BBB5-49C0-BF9A-BACEFAD8520D@yesql.se>
 <5C27CF6F-5BD3-4639-AAA6-73465595B6DF@vmware.com>
 <AC495586-B79D-45DB-A2ED-2E4FC24ED3C7@vmware.com>
In-Reply-To: <AC495586-B79D-45DB-A2ED-2E4FC24ED3C7@vmware.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: multipart/mixed;
	boundary="_002_FBA0C3653B194E4098F48B0064DB7127vmwarecom_"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 
 e354e99b-be20-45c2-bf54-08d887581341
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Nov 2020 22:12:42.0483
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b39138ca-3cee-4b4a-a4d6-cd83d9dd62f0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 
 GjwNjFO+cbW7bFhJCxVyiekVWfGcqpkxj6XCZ1K/Gj20gUbSJ3sXrYuCfS/swrrEBlJrihat8fdkNWAr/o0iDg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR05MB2798
List-Id: <pgsql-hackers.lists.postgresql.org>
List-Help: <https://lists.postgresql.org/manage/>
List-Subscribe: <https://lists.postgresql.org/manage/>
List-Post: <mailto:pgsql-hackers@lists.postgresql.org>
List-Owner: <mailto:pgsql-hackers-owner@lists.postgresql.org>
List-Archive: <https://www.postgresql.org/list/pgsql-hackers>
Precedence: bulk

--_002_FBA0C3653B194E4098F48B0064DB7127vmwarecom_
Content-Type: text/plain; charset="us-ascii"
Content-ID: <15EA6B6039E0754EBF17D27EB16AF539@namprd05.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable

On Nov 11, 2020, at 10:57 AM, Jacob Champion <pchampion@vmware.com> wrote:
>=20
> False alarm -- the stderr debugging I'd added in to track down the
> assertion tripped up the "no stderr" tests. Zero failing tests now.

I took a look at the OpenSSL interop problems you mentioned upthread. I
don't see a hang like you did, but I do see a PR_IO_TIMEOUT_ERROR during
connection.

I think pgtls_read() needs to treat PR_IO_TIMEOUT_ERROR as if no bytes
were read, in order to satisfy its API. There was some discussion on
this upthread:

On Oct 27, 2020, at 1:07 PM, Daniel Gustafsson <daniel@yesql.se> wrote:
>=20
> On 20 Oct 2020, at 21:15, Andres Freund <andres@anarazel.de> wrote:
>>=20
>>> +			case PR_IO_TIMEOUT_ERROR:
>>> +				break;
>>=20
>> What does this mean? We'll return with a 0 errno here, right? When is
>> this case reachable?
>=20
> It should, AFAICT, only be reachable when PR_Recv is used with a timeout =
which
> we don't do.  It mentioned somewhere that it had happened in no-wait call=
s due
> to a bug, but I fail to find that reference now.  Either way, I've remove=
d it
> to fall into the default error handling which now sets errno correctly as=
 that
> was a paddle short here.

PR_IO_TIMEOUT_ERROR is definitely returned in no-wait calls on my
machine. It doesn't look like the PR_Recv() API has a choice -- if
there's no data, it can't return a positive integer, and returning zero
means that the socket has been disconnected. So -1 with a timeout error
is the only option.

I'm not completely sure why this is exposed so easily with an OpenSSL
server -- I'm guessing the implementation slices up its packets
differently on the wire, causing a read event before NSS is able to
decrypt a full record -- but it's worth noting that this case also shows
up during NSS-to-NSS psql connections, when handling notifications at
the end of every query. PQconsumeInput() reports a hard failure with the
current implementation, but its return value is ignored by
PrintNotifications(). Otherwise this probably would have showed up
earlier.

(What's the best way to test this case? Are there lower-level tests for
the protocol/network layer somewhere that I'm missing?)=20

While patching this case, I also noticed that pgtls_read() doesn't call
SOCK_ERRNO_SET() for the disconnection case. That is also in the
attached patch.

--Jacob


--_002_FBA0C3653B194E4098F48B0064DB7127vmwarecom_
Content-Type: application/octet-stream;
	name="nss-handle-timeouts-and-disconnections-in-pgtls_read.patch"
Content-Description: 
 nss-handle-timeouts-and-disconnections-in-pgtls_read.patch
Content-Disposition: attachment;
	filename="nss-handle-timeouts-and-disconnections-in-pgtls_read.patch";
	size=2010; creation-date="Thu, 12 Nov 2020 22:12:41 GMT";
	modification-date="Thu, 12 Nov 2020 22:12:41 GMT"
Content-ID: <04F1DBDFE9F0FB48B7A8A664AEC1B2D6@namprd05.prod.outlook.com>
Content-Transfer-Encoding: base64

RnJvbSA0YmM1ZGM2MWU4YmU0OTBiMDEzYmU1ZjliMzMyNmZhYWUyZGM5OWM2IE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBKYWNvYiBDaGFtcGlvbiA8cGNoYW1waW9uQHZtd2FyZS5jb20+
CkRhdGU6IFRodSwgMTIgTm92IDIwMjAgMTI6MDI6MzkgLTA4MDAKU3ViamVjdDogW1BBVENIXSBu
c3M6IGhhbmRsZSB0aW1lb3V0cyBhbmQgZGlzY29ubmVjdGlvbnMgaW4gcGd0bHNfcmVhZAoKUFJf
SU9fVElNRU9VVF9FUlJPUiB3YXMgYmVpbmcgdHJlYXRlZCBhcyBhIGhhcmQgZXJyb3IsIGJ1dCBz
aW5jZSB3ZSdyZQpleHBsaWNpdGx5IGFza2luZyBQUl9SZWN2KCkgbm90IHRvIHdhaXQsIHdlIHdh
bnQgdG8gdHJlYXQgaXQgYXMgInplcm8KYnl0ZXMgcmVhZCIgaW5zdGVhZC4KClRoaXMgY2FzZSBo
YXBwZW5zIHJlbGF0aXZlbHkgb2Z0ZW4gaW4gcHNxbCwgZHVyaW5nIFBRY29uc3VtZUlucHV0KCkg
LS0KYnV0IHRoZSByZXR1cm4gdmFsdWUgb2YgUFFjb25zdW1lSW5wdXQoKSBpcyBiZWluZyBpZ25v
cmVkIGR1cmluZwpub3RpZmljYXRpb24gaGFuZGxpbmcsIHdoaWNoIG1hc2tzIHRoZSBwcm9ibGVt
LiBUaGlzIGFsc28gc2VlbXMgdG8gZml4CmludGVyb3BlcmFiaWxpdHkgd2l0aCBPcGVuU1NMIHNl
cnZlcnMgb24gbXkgbWFjaGluZSwgcG9zc2libHkgZHVlIHRvCmRpZmZlcmVuY2VzIGluIG1lc3Nh
Z2UgZnJhZ21lbnRhdGlvbiBiZXR3ZWVuIHRoZSBpbXBsZW1lbnRhdGlvbnM/CgpBbHNvIGVuc3Vy
ZSB0aGF0IFNPQ0tfRVJSTk9fU0VUKCkgaXMgY2FsbGVkIGluIHRoZSBkaXNjb25uZWN0aW9uIGNh
c2U7CnJlYWRfZXJybm8gd2FzIGJlaW5nIHNldCBidXQgbm90IHVzZWQuCi0tLQogc3JjL2ludGVy
ZmFjZXMvbGlicHEvZmUtc2VjdXJlLW5zcy5jIHwgMTkgKysrKysrKysrKysrKy0tLS0tLQogMSBm
aWxlIGNoYW5nZWQsIDEzIGluc2VydGlvbnMoKyksIDYgZGVsZXRpb25zKC0pCgpkaWZmIC0tZ2l0
IGEvc3JjL2ludGVyZmFjZXMvbGlicHEvZmUtc2VjdXJlLW5zcy5jIGIvc3JjL2ludGVyZmFjZXMv
bGlicHEvZmUtc2VjdXJlLW5zcy5jCmluZGV4IDMwOTBmMzdmNWUuLjhmYzhlMTM4ZTIgMTAwNjQ0
Ci0tLSBhL3NyYy9pbnRlcmZhY2VzL2xpYnBxL2ZlLXNlY3VyZS1uc3MuYworKysgYi9zcmMvaW50
ZXJmYWNlcy9saWJwcS9mZS1zZWN1cmUtbnNzLmMKQEAgLTQyOSwxNSArNDI5LDE5IEBAIHBndGxz
X3JlYWQoUEdjb25uICpjb25uLCB2b2lkICpwdHIsIHNpemVfdCBsZW4pCiAJaWYgKG5yZWFkID09
IDApCiAJewogCQlyZWFkX2Vycm5vID0gRUNPTk5SRVNFVDsKLQkJcmV0dXJuIC0xOworCQlucmVh
ZCA9IC0xOwogCX0KLQotCWlmIChucmVhZCA9PSAtMSkKKwllbHNlIGlmIChucmVhZCA9PSAtMSkK
IAl7CiAJCXN0YXR1cyA9IFBSX0dldEVycm9yKCk7CiAKIAkJc3dpdGNoIChzdGF0dXMpCiAJCXsK
KwkJCWNhc2UgUFJfSU9fVElNRU9VVF9FUlJPUjoKKwkJCQkvKiBObyBkYXRhIGF2YWlsYWJsZSB5
ZXQuICovCisJCQkJbnJlYWQgPSAwOworCQkJCWJyZWFrOworCiAJCQljYXNlIFBSX1dPVUxEX0JM
T0NLX0VSUk9SOgogCQkJCXJlYWRfZXJybm8gPSBFV09VTERCTE9DSzsKIAkJCQlicmVhazsKQEAg
LTQ1Niw5ICs0NjAsMTIgQEAgcGd0bHNfcmVhZChQR2Nvbm4gKmNvbm4sIHZvaWQgKnB0ciwgc2l6
ZV90IGxlbikKIAkJCQlicmVhazsKIAkJfQogCi0JCXByaW50ZlBRRXhwQnVmZmVyKCZjb25uLT5l
cnJvck1lc3NhZ2UsCi0JCQkJCQkgIGxpYnBxX2dldHRleHQoIlRMUyByZWFkIGVycm9yOiAlcyIp
LAotCQkJCQkJICBwZ19TU0xlcnJtZXNzYWdlKHN0YXR1cykpOworCQlpZiAobnJlYWQgPT0gLTEp
CisJCXsKKwkJCXByaW50ZlBRRXhwQnVmZmVyKCZjb25uLT5lcnJvck1lc3NhZ2UsCisJCQkJCQkJ
ICBsaWJwcV9nZXR0ZXh0KCJUTFMgcmVhZCBlcnJvcjogJXMiKSwKKwkJCQkJCQkgIHBnX1NTTGVy
cm1lc3NhZ2Uoc3RhdHVzKSk7CisJCX0KIAl9CiAKIAlTT0NLX0VSUk5PX1NFVChyZWFkX2Vycm5v
KTsKLS0gCjIuMjQuMQoK

--_002_FBA0C3653B194E4098F48B0064DB7127vmwarecom_--