Sloppy SSPI error reporting code
While looking at fe-auth.c I noticed quite a few places that weren't
bothering to make error messages localizable (ie, missing libpq_gettext
calls), and/or were failing to add a trailing newline as expected in
libpq error messages. Perhaps these are intentional but I doubt it.
Most of the instances seemed to be SSPI-related.
I have no intention of fixing these myself, but whoever committed that
code should take a second look.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Jan 10, 2015 at 02:53:13PM -0500, Tom Lane wrote:
While looking at fe-auth.c I noticed quite a few places that weren't
bothering to make error messages localizable (ie, missing libpq_gettext
calls), and/or were failing to add a trailing newline as expected in
libpq error messages. Perhaps these are intentional but I doubt it.
Most of the instances seemed to be SSPI-related.I have no intention of fixing these myself, but whoever committed that
code should take a second look.
I looked through that file and only found two cases; patch attached.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
Attachments:
libpq.difftext/x-diff; charset=us-asciiDownload
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
new file mode 100644
index 8927df4..c267f72
*** a/src/interfaces/libpq/fe-auth.c
--- b/src/interfaces/libpq/fe-auth.c
*************** pg_SSPI_continue(PGconn *conn)
*** 333,339 ****
* authentication. Keep check in case it shows up with other
* authentication methods later.
*/
! printfPQExpBuffer(&conn->errorMessage, "SSPI returned invalid number of output buffers\n");
return STATUS_ERROR;
}
--- 333,339 ----
* authentication. Keep check in case it shows up with other
* authentication methods later.
*/
! printfPQExpBuffer(&conn->errorMessage, libpq_gettext("SSPI returned invalid number of output buffers\n"));
return STATUS_ERROR;
}
*************** pg_fe_sendauth(AuthRequest areq, PGconn
*** 691,697 ****
if (pg_password_sendauth(conn, conn->pgpass, areq) != STATUS_OK)
{
printfPQExpBuffer(&conn->errorMessage,
! "fe_sendauth: error sending password authentication\n");
return STATUS_ERROR;
}
break;
--- 691,697 ----
if (pg_password_sendauth(conn, conn->pgpass, areq) != STATUS_OK)
{
printfPQExpBuffer(&conn->errorMessage,
! libpq_gettext("fe_sendauth: error sending password authentication\n"));
return STATUS_ERROR;
}
break;
On Wed, Apr 01, 2015 at 10:49:01PM -0400, Bruce Momjian wrote:
On Sat, Jan 10, 2015 at 02:53:13PM -0500, Tom Lane wrote:
While looking at fe-auth.c I noticed quite a few places that weren't
bothering to make error messages localizable (ie, missing libpq_gettext
calls), and/or were failing to add a trailing newline as expected in
libpq error messages. Perhaps these are intentional but I doubt it.
Most of the instances seemed to be SSPI-related.I have no intention of fixing these myself, but whoever committed that
code should take a second look.I looked through that file and only found two cases; patch attached.
Tom mentioned newline omissions, which you'll find in pg_SSPI_error().
! printfPQExpBuffer(&conn->errorMessage, libpq_gettext("SSPI returned invalid number of output buffers\n"));
! libpq_gettext("fe_sendauth: error sending password authentication\n"));
The first message is too internals-focused for a translation to be useful. If
it were in the backend, we would use elog() and not translate. The second is
a non-actionable message painting over a wide range of specific failures;
translating it would just put lipstick on the pig.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Apr 2, 2015 at 01:44:59AM -0400, Noah Misch wrote:
On Wed, Apr 01, 2015 at 10:49:01PM -0400, Bruce Momjian wrote:
On Sat, Jan 10, 2015 at 02:53:13PM -0500, Tom Lane wrote:
While looking at fe-auth.c I noticed quite a few places that weren't
bothering to make error messages localizable (ie, missing libpq_gettext
calls), and/or were failing to add a trailing newline as expected in
libpq error messages. Perhaps these are intentional but I doubt it.
Most of the instances seemed to be SSPI-related.I have no intention of fixing these myself, but whoever committed that
code should take a second look.I looked through that file and only found two cases; patch attached.
Tom mentioned newline omissions, which you'll find in pg_SSPI_error().
Oh, I accidentally saw the backend version of that function, which
looked fine. I have attached a patch for that.
! printfPQExpBuffer(&conn->errorMessage, libpq_gettext("SSPI returned invalid number of output buffers\n"));
! libpq_gettext("fe_sendauth: error sending password authentication\n"));The first message is too internals-focused for a translation to be useful. If
it were in the backend, we would use elog() and not translate. The second is
a non-actionable message painting over a wide range of specific failures;
translating it would just put lipstick on the pig.
OK.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
Attachments:
sspi.difftext/x-diff; charset=us-asciiDownload
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
new file mode 100644
index 8927df4..08cc906
*** a/src/interfaces/libpq/fe-auth.c
--- b/src/interfaces/libpq/fe-auth.c
*************** pg_SSPI_error(PGconn *conn, const char *
*** 236,245 ****
if (FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM, NULL, r, 0,
sysmsg, sizeof(sysmsg), NULL) == 0)
! printfPQExpBuffer(&conn->errorMessage, "%s: SSPI error %x",
mprefix, (unsigned int) r);
else
! printfPQExpBuffer(&conn->errorMessage, "%s: %s (%x)",
mprefix, sysmsg, (unsigned int) r);
}
--- 236,245 ----
if (FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM, NULL, r, 0,
sysmsg, sizeof(sysmsg), NULL) == 0)
! printfPQExpBuffer(&conn->errorMessage, "%s: SSPI error %x\n",
mprefix, (unsigned int) r);
else
! printfPQExpBuffer(&conn->errorMessage, "%s: %s (%x)\n",
mprefix, sysmsg, (unsigned int) r);
}
On Thu, Apr 02, 2015 at 07:31:52AM -0400, Bruce Momjian wrote:
On Thu, Apr 2, 2015 at 01:44:59AM -0400, Noah Misch wrote:
On Wed, Apr 01, 2015 at 10:49:01PM -0400, Bruce Momjian wrote:
On Sat, Jan 10, 2015 at 02:53:13PM -0500, Tom Lane wrote:
While looking at fe-auth.c I noticed quite a few places that weren't
bothering to make error messages localizable (ie, missing libpq_gettext
calls), and/or were failing to add a trailing newline as expected in
libpq error messages. Perhaps these are intentional but I doubt it.
Most of the instances seemed to be SSPI-related.I have no intention of fixing these myself, but whoever committed that
code should take a second look.I looked through that file and only found two cases; patch attached.
Tom mentioned newline omissions, which you'll find in pg_SSPI_error().
Oh, I accidentally saw the backend version of that function, which
looked fine. I have attached a patch for that.
That patch looks reasonable.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Apr 3, 2015 at 08:32:08PM -0400, Noah Misch wrote:
On Thu, Apr 02, 2015 at 07:31:52AM -0400, Bruce Momjian wrote:
On Thu, Apr 2, 2015 at 01:44:59AM -0400, Noah Misch wrote:
On Wed, Apr 01, 2015 at 10:49:01PM -0400, Bruce Momjian wrote:
On Sat, Jan 10, 2015 at 02:53:13PM -0500, Tom Lane wrote:
While looking at fe-auth.c I noticed quite a few places that weren't
bothering to make error messages localizable (ie, missing libpq_gettext
calls), and/or were failing to add a trailing newline as expected in
libpq error messages. Perhaps these are intentional but I doubt it.
Most of the instances seemed to be SSPI-related.I have no intention of fixing these myself, but whoever committed that
code should take a second look.I looked through that file and only found two cases; patch attached.
Tom mentioned newline omissions, which you'll find in pg_SSPI_error().
Oh, I accidentally saw the backend version of that function, which
looked fine. I have attached a patch for that.That patch looks reasonable.
Patch applied. Thanks.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers