Re: [BUGS] BUG #4680: Server crashed if using wrong (mismatch) conversion functions

I wrote:
> In any case, that's orthogonal to the part that I was focusing on,
> which was to try to prevent error recursion as a result of trouble
> in the encoding conversion subsystem.  It looks like we could do that
> with some additional hacking in send_message_to_frontend() to avoid
> conversion, as well as translation, when in_error_recursion_trouble()
> is true.  Your point about there possibly being non-ASCII user-inserted
> data in the message is a bit troubling, but for the cases where
> recursion is actually occurring I don't think that that will happen.

Here is a proposed patch that does this.  It largely reverts my patch
of 2008-10-27 in favor of a more general policy that says that *all*
localization of error messages is disabled once we get into error
recursion trouble.  Having done that, we can reasonably assume that
the error message text is 7-bit ASCII, and therefore bypass encoding
conversion as well.  This fixes the example reported in bug #4680
(even without the subsequent patch to prevent that case from arising),
and it still prevents the cases that my previous patch was meant to
deal with.

Comments, objections?

            regards, tom lane

Index: src/backend/libpq/pqformat.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/libpq/pqformat.c,v
retrieving revision 1.48
diff -c -r1.48 pqformat.c
*** src/backend/libpq/pqformat.c    1 Jan 2009 17:23:42 -0000    1.48
--- src/backend/libpq/pqformat.c    2 Mar 2009 19:13:12 -0000
***************
*** 41,46 ****
--- 41,47 ----
   *        pq_sendcountedtext - append a counted text string (with character set conversion)
   *        pq_sendtext        - append a text string (with conversion)
   *        pq_sendstring    - append a null-terminated text string (with conversion)
+  *        pq_send_raw_string - append a null-terminated text string (without conversion)
   *        pq_endmessage    - send the completed message to the frontend
   * Note: it is also possible to append data to the StringInfo buffer using
   * the regular StringInfo routines, but this is discouraged since required
***************
*** 184,190 ****
  pq_sendstring(StringInfo buf, const char *str)
  {
      int            slen = strlen(str);
-
      char       *p;

      p = pg_server_to_client(str, slen);
--- 185,190 ----
***************
*** 199,204 ****
--- 199,221 ----
  }

  /* --------------------------------
+  *        pq_send_raw_string    - append a null-terminated text string (without conversion)
+  *
+  * This function intentionally bypasses encoding conversion; it should be used
+  * only in very specialized cases, preferable only when the given string is
+  * known to be just 7-bit ASCII.
+  *
+  * NB: passed text string must be null-terminated, and so is the data
+  * sent to the frontend.
+  * --------------------------------
+  */
+ void
+ pq_send_raw_string(StringInfo buf, const char *str)
+ {
+     appendBinaryStringInfo(buf, str, strlen(str) + 1);
+ }
+
+ /* --------------------------------
   *        pq_sendint        - append a binary integer to a StringInfo buffer
   * --------------------------------
   */
Index: src/backend/utils/error/elog.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/error/elog.c,v
retrieving revision 1.212
diff -c -r1.212 elog.c
*** src/backend/utils/error/elog.c    19 Jan 2009 15:34:23 -0000    1.212
--- src/backend/utils/error/elog.c    2 Mar 2009 19:13:13 -0000
***************
*** 72,77 ****
--- 72,80 ----
  #include "utils/ps_status.h"


+ #undef _
+ #define _(x) err_gettext(x)
+
  /* Global variables */
  ErrorContextCallback *error_context_stack = NULL;

***************
*** 165,170 ****
--- 168,192 ----
  }

  /*
+  * One of those fallback steps is to stop trying to localize the error
+  * message, since there's a significant probability that that's exactly
+  * what's causing the recursion.
+  */
+ static inline const char *
+ err_gettext(const char *str)
+ {
+ #ifdef ENABLE_NLS
+     if (in_error_recursion_trouble())
+         return str;
+     else
+         return gettext(str);
+ #else
+     return str;
+ #endif
+ }
+
+
+ /*
   * errstart --- begin an error-reporting cycle
   *
   * Create a stack entry and store the given parameters in it.  Subsequently,
***************
*** 631,637 ****
          char           *fmtbuf; \
          StringInfoData    buf; \
          /* Internationalize the error format string */ \
!         if (translateit) \
              fmt = dgettext(edata->domain, fmt); \
          /* Expand %m in format string */ \
          fmtbuf = expand_fmt_string(fmt, edata); \
--- 653,659 ----
          char           *fmtbuf; \
          StringInfoData    buf; \
          /* Internationalize the error format string */ \
!         if (translateit && !in_error_recursion_trouble()) \
              fmt = dgettext(edata->domain, fmt); \
          /* Expand %m in format string */ \
          fmtbuf = expand_fmt_string(fmt, edata); \
***************
*** 2137,2143 ****
          }
          else
          {
!             char       *msg = _("Not safe to send CSV data\n");

              write(fileno(stderr), msg, strlen(msg));
              if (!(Log_destination & LOG_DESTINATION_STDERR) &&
--- 2159,2165 ----
          }
          else
          {
!             const char *msg = _("Not safe to send CSV data\n");

              write(fileno(stderr), msg, strlen(msg));
              if (!(Log_destination & LOG_DESTINATION_STDERR) &&
***************
*** 2190,2195 ****
--- 2212,2237 ----


  /*
+  * Append a text string to the error report being built for the client.
+  *
+  * This is ordinarily identical to pq_sendstring(), but if we are in
+  * error recursion trouble we skip encoding conversion, because of the
+  * possibility that the problem is a failure in the encoding conversion
+  * subsystem itself.  Code elsewhere should ensure that the passed-in
+  * strings will be plain 7-bit ASCII, and thus not in need of conversion,
+  * in such cases.  (In particular, we disable localization of error messages
+  * to help ensure that's true.)
+  */
+ static void
+ err_sendstring(StringInfo buf, const char *str)
+ {
+     if (in_error_recursion_trouble())
+         pq_send_raw_string(buf, str);
+     else
+         pq_sendstring(buf, str);
+ }
+
+ /*
   * Write error report to client
   */
  static void
***************
*** 2208,2214 ****
          int            i;

          pq_sendbyte(&msgbuf, PG_DIAG_SEVERITY);
!         pq_sendstring(&msgbuf, error_severity(edata->elevel));

          /* unpack MAKE_SQLSTATE code */
          ssval = edata->sqlerrcode;
--- 2250,2256 ----
          int            i;

          pq_sendbyte(&msgbuf, PG_DIAG_SEVERITY);
!         err_sendstring(&msgbuf, error_severity(edata->elevel));

          /* unpack MAKE_SQLSTATE code */
          ssval = edata->sqlerrcode;
***************
*** 2220,2238 ****
          tbuf[i] = '\0';

          pq_sendbyte(&msgbuf, PG_DIAG_SQLSTATE);
!         pq_sendstring(&msgbuf, tbuf);

          /* M field is required per protocol, so always send something */
          pq_sendbyte(&msgbuf, PG_DIAG_MESSAGE_PRIMARY);
          if (edata->message)
!             pq_sendstring(&msgbuf, edata->message);
          else
!             pq_sendstring(&msgbuf, _("missing error text"));

          if (edata->detail)
          {
              pq_sendbyte(&msgbuf, PG_DIAG_MESSAGE_DETAIL);
!             pq_sendstring(&msgbuf, edata->detail);
          }

          /* detail_log is intentionally not used here */
--- 2262,2280 ----
          tbuf[i] = '\0';

          pq_sendbyte(&msgbuf, PG_DIAG_SQLSTATE);
!         err_sendstring(&msgbuf, tbuf);

          /* M field is required per protocol, so always send something */
          pq_sendbyte(&msgbuf, PG_DIAG_MESSAGE_PRIMARY);
          if (edata->message)
!             err_sendstring(&msgbuf, edata->message);
          else
!             err_sendstring(&msgbuf, _("missing error text"));

          if (edata->detail)
          {
              pq_sendbyte(&msgbuf, PG_DIAG_MESSAGE_DETAIL);
!             err_sendstring(&msgbuf, edata->detail);
          }

          /* detail_log is intentionally not used here */
***************
*** 2240,2291 ****
          if (edata->hint)
          {
              pq_sendbyte(&msgbuf, PG_DIAG_MESSAGE_HINT);
!             pq_sendstring(&msgbuf, edata->hint);
          }

          if (edata->context)
          {
              pq_sendbyte(&msgbuf, PG_DIAG_CONTEXT);
!             pq_sendstring(&msgbuf, edata->context);
          }

          if (edata->cursorpos > 0)
          {
              snprintf(tbuf, sizeof(tbuf), "%d", edata->cursorpos);
              pq_sendbyte(&msgbuf, PG_DIAG_STATEMENT_POSITION);
!             pq_sendstring(&msgbuf, tbuf);
          }

          if (edata->internalpos > 0)
          {
              snprintf(tbuf, sizeof(tbuf), "%d", edata->internalpos);
              pq_sendbyte(&msgbuf, PG_DIAG_INTERNAL_POSITION);
!             pq_sendstring(&msgbuf, tbuf);
          }

          if (edata->internalquery)
          {
              pq_sendbyte(&msgbuf, PG_DIAG_INTERNAL_QUERY);
!             pq_sendstring(&msgbuf, edata->internalquery);
          }

          if (edata->filename)
          {
              pq_sendbyte(&msgbuf, PG_DIAG_SOURCE_FILE);
!             pq_sendstring(&msgbuf, edata->filename);
          }

          if (edata->lineno > 0)
          {
              snprintf(tbuf, sizeof(tbuf), "%d", edata->lineno);
              pq_sendbyte(&msgbuf, PG_DIAG_SOURCE_LINE);
!             pq_sendstring(&msgbuf, tbuf);
          }

          if (edata->funcname)
          {
              pq_sendbyte(&msgbuf, PG_DIAG_SOURCE_FUNCTION);
!             pq_sendstring(&msgbuf, edata->funcname);
          }

          pq_sendbyte(&msgbuf, '\0');        /* terminator */
--- 2282,2333 ----
          if (edata->hint)
          {
              pq_sendbyte(&msgbuf, PG_DIAG_MESSAGE_HINT);
!             err_sendstring(&msgbuf, edata->hint);
          }

          if (edata->context)
          {
              pq_sendbyte(&msgbuf, PG_DIAG_CONTEXT);
!             err_sendstring(&msgbuf, edata->context);
          }

          if (edata->cursorpos > 0)
          {
              snprintf(tbuf, sizeof(tbuf), "%d", edata->cursorpos);
              pq_sendbyte(&msgbuf, PG_DIAG_STATEMENT_POSITION);
!             err_sendstring(&msgbuf, tbuf);
          }

          if (edata->internalpos > 0)
          {
              snprintf(tbuf, sizeof(tbuf), "%d", edata->internalpos);
              pq_sendbyte(&msgbuf, PG_DIAG_INTERNAL_POSITION);
!             err_sendstring(&msgbuf, tbuf);
          }

          if (edata->internalquery)
          {
              pq_sendbyte(&msgbuf, PG_DIAG_INTERNAL_QUERY);
!             err_sendstring(&msgbuf, edata->internalquery);
          }

          if (edata->filename)
          {
              pq_sendbyte(&msgbuf, PG_DIAG_SOURCE_FILE);
!             err_sendstring(&msgbuf, edata->filename);
          }

          if (edata->lineno > 0)
          {
              snprintf(tbuf, sizeof(tbuf), "%d", edata->lineno);
              pq_sendbyte(&msgbuf, PG_DIAG_SOURCE_LINE);
!             err_sendstring(&msgbuf, tbuf);
          }

          if (edata->funcname)
          {
              pq_sendbyte(&msgbuf, PG_DIAG_SOURCE_FUNCTION);
!             err_sendstring(&msgbuf, edata->funcname);
          }

          pq_sendbyte(&msgbuf, '\0');        /* terminator */
***************
*** 2316,2322 ****

          appendStringInfoChar(&buf, '\n');

!         pq_sendstring(&msgbuf, buf.data);

          pfree(buf.data);
      }
--- 2358,2364 ----

          appendStringInfoChar(&buf, '\n');

!         err_sendstring(&msgbuf, buf.data);

          pfree(buf.data);
      }
***************
*** 2430,2439 ****

  /*
   * error_severity --- get localized string representing elevel
-  *
-  * Note: in an error recursion situation, we stop localizing the tags
-  * for ERROR and above.  This is necessary because the problem might be
-  * failure to convert one of these strings to the client encoding.
   */
  static const char *
  error_severity(int elevel)
--- 2472,2477 ----
***************
*** 2463,2484 ****
              prefix = _("WARNING");
              break;
          case ERROR:
!             if (in_error_recursion_trouble())
!                 prefix = "ERROR";
!             else
!                 prefix = _("ERROR");
              break;
          case FATAL:
!             if (in_error_recursion_trouble())
!                 prefix = "FATAL";
!             else
!                 prefix = _("FATAL");
              break;
          case PANIC:
!             if (in_error_recursion_trouble())
!                 prefix = "PANIC";
!             else
!                 prefix = _("PANIC");
              break;
          default:
              prefix = "???";
--- 2501,2513 ----
              prefix = _("WARNING");
              break;
          case ERROR:
!             prefix = _("ERROR");
              break;
          case FATAL:
!             prefix = _("FATAL");
              break;
          case PANIC:
!             prefix = _("PANIC");
              break;
          default:
              prefix = "???";
Index: src/backend/utils/mb/wchar.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/mb/wchar.c,v
retrieving revision 1.71
diff -c -r1.71 wchar.c
*** src/backend/utils/mb/wchar.c    10 Feb 2009 19:29:39 -0000    1.71
--- src/backend/utils/mb/wchar.c    2 Mar 2009 19:13:13 -0000
***************
*** 1636,1660 ****
      for (j = 0; j < jlimit; j++)
          p += sprintf(p, "%02x", (unsigned char) mbstr[j]);

!     /*
!      * In an error recursion situation, don't try to translate the message.
!      * This gets us out of trouble if the problem is failure to convert
!      * this very message (after translation) to the client encoding.
!      */
!     if (in_error_recursion_trouble())
!         ereport(ERROR,
!                 (errcode(ERRCODE_UNTRANSLATABLE_CHARACTER),
!                  errmsg_internal("character 0x%s of encoding \"%s\" has no equivalent in \"%s\"",
!                                  buf,
!                                  pg_enc2name_tbl[src_encoding].name,
!                                  pg_enc2name_tbl[dest_encoding].name)));
!     else
!         ereport(ERROR,
!                 (errcode(ERRCODE_UNTRANSLATABLE_CHARACTER),
!                  errmsg("character 0x%s of encoding \"%s\" has no equivalent in \"%s\"",
!                         buf,
!                         pg_enc2name_tbl[src_encoding].name,
!                         pg_enc2name_tbl[dest_encoding].name)));
  }

  #endif
--- 1636,1647 ----
      for (j = 0; j < jlimit; j++)
          p += sprintf(p, "%02x", (unsigned char) mbstr[j]);

!     ereport(ERROR,
!             (errcode(ERRCODE_UNTRANSLATABLE_CHARACTER),
!              errmsg("character 0x%s of encoding \"%s\" has no equivalent in \"%s\"",
!                     buf,
!                     pg_enc2name_tbl[src_encoding].name,
!                     pg_enc2name_tbl[dest_encoding].name)));
  }

  #endif
Index: src/include/libpq/pqformat.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/libpq/pqformat.h,v
retrieving revision 1.27
diff -c -r1.27 pqformat.h
*** src/include/libpq/pqformat.h    1 Jan 2009 17:23:59 -0000    1.27
--- src/include/libpq/pqformat.h    2 Mar 2009 19:13:13 -0000
***************
*** 22,27 ****
--- 22,28 ----
                     bool countincludesself);
  extern void pq_sendtext(StringInfo buf, const char *str, int slen);
  extern void pq_sendstring(StringInfo buf, const char *str);
+ extern void pq_send_raw_string(StringInfo buf, const char *str);
  extern void pq_sendint(StringInfo buf, int i, int b);
  extern void pq_sendint64(StringInfo buf, int64 i);
  extern void pq_sendfloat4(StringInfo buf, float4 f);