Re: BUG #4680: Server crashed if using wrong (mismatch) conversion functions - Mailing list pgsql-bugs

Denis Afonin wrote:
> test=# CREATE DEFAULT CONVERSION test1 FOR 'LATIN1' TO 'KOI8' FROM
> ascii_to_mic;
> CREATE CONVERSION
> test=# CREATE DEFAULT CONVERSION test2 FOR 'KOI8' TO 'LATIN1' FROM
> mic_to_ascii;
> CREATE CONVERSION
> test=# set client_encoding to 'LATIN1';
> server closed the connection unexpectedly
>     This probably means the server terminated abnormally
>     before or while processing the request.
> Соединение с сервером было потеряно.
> Попытка переустановить: Безуспешно.

Hmm, this seems to be another variant of the recursive error issue fixed
earlier:
http://archives.postgresql.org/message-id/20081027193722.283B67545A4@cvs.postgresql.org

Only this time the problem the error doesn't occur in translation, but
in encoding conversion. We could doo something similar to what we did
before with the translation, and try not to call conversion function in
case of a recursive error. However, sending an error message to the
client in wrong encoding is not as sane as sending it untranslated.

I think we should instead try to break the PANIC cycle. If we exceed
ERRORDATA_STACK_SIZE, and we're already PANICing, we should just die
immediately instead of throwing another PANIC about exceeding the stack
size. The attached patch does that.

However, a more serious issue is that a regular user can do that and
bring down the whole system. I suggest that we make "CREATE [DEFAULT]
CONVERSION" to call the conversion function with a empty string, to
check that it is in fact capable of doing the conversion. See 2nd
attached patch. This is a usability improvement too, as you

--
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index a33c94e..24230e5 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -149,6 +149,39 @@ static void write_csvlog(ErrorData *edata);
 static void setup_formatted_log_time(void);
 static void setup_formatted_start_time(void);

+/*
+ * Increment errordata stack depth. Return pointer to the new topmost
+ * entry. The caller should fill it in.
+ */
+static ErrorData *
+push_errordata(void)
+{
+    if (++errordata_stack_depth >= ERRORDATA_STACK_SIZE)
+    {
+        /*
+         * Wups, stack not big enough.    We treat this as a PANIC condition
+         * because it suggests an infinite loop of errors during error
+         * recovery. If we had PANIC'd already, just die now, because a new
+         * ereport(PANIC) might fail again and cause infinite recursion.
+         */
+        if (errordata[ERRORDATA_STACK_SIZE - 1].elevel >= PANIC)
+        {
+            ImmediateInterruptOK = false;
+            fflush(stdout);
+            fflush(stderr);
+            abort();
+        }
+        errordata_stack_depth = -1;        /* make room on stack */
+
+        /*
+         * Note that the message is intentionally not localized, else
+         * failure to convert it to client encoding could cause further
+         * recursion.
+         */
+        ereport(PANIC, (errmsg_internal("ERRORDATA_STACK_SIZE exceeded")));
+    }
+    return &errordata[errordata_stack_depth];
+}

 /*
  * in_error_recursion_trouble --- are we at risk of infinite error recursion?
@@ -287,19 +320,9 @@ errstart(int elevel, const char *filename, int lineno,
             debug_query_string = NULL;
         }
     }
-    if (++errordata_stack_depth >= ERRORDATA_STACK_SIZE)
-    {
-        /*
-         * Wups, stack not big enough.    We treat this as a PANIC condition
-         * because it suggests an infinite loop of errors during error
-         * recovery.
-         */
-        errordata_stack_depth = -1;        /* make room on stack */
-        ereport(PANIC, (errmsg_internal("ERRORDATA_STACK_SIZE exceeded")));
-    }

     /* Initialize data for this error frame */
-    edata = &errordata[errordata_stack_depth];
+    edata = push_errordata();
     MemSet(edata, 0, sizeof(ErrorData));
     edata->elevel = elevel;
     edata->output_to_server = output_to_server;
@@ -971,20 +994,7 @@ elog_start(const char *filename, int lineno, const char *funcname)
 {
     ErrorData  *edata;

-    if (++errordata_stack_depth >= ERRORDATA_STACK_SIZE)
-    {
-        /*
-         * Wups, stack not big enough.    We treat this as a PANIC condition
-         * because it suggests an infinite loop of errors during error
-         * recovery.  Note that the message is intentionally not localized,
-         * else failure to convert it to client encoding could cause further
-         * recursion.
-         */
-        errordata_stack_depth = -1;        /* make room on stack */
-        ereport(PANIC, (errmsg_internal("ERRORDATA_STACK_SIZE exceeded")));
-    }
-
-    edata = &errordata[errordata_stack_depth];
+    edata = push_errordata();
     edata->filename = filename;
     edata->lineno = lineno;
     edata->funcname = funcname;
@@ -1165,18 +1175,7 @@ ReThrowError(ErrorData *edata)
     recursion_depth++;
     MemoryContextSwitchTo(ErrorContext);

-    if (++errordata_stack_depth >= ERRORDATA_STACK_SIZE)
-    {
-        /*
-         * Wups, stack not big enough.    We treat this as a PANIC condition
-         * because it suggests an infinite loop of errors during error
-         * recovery.
-         */
-        errordata_stack_depth = -1;        /* make room on stack */
-        ereport(PANIC, (errmsg_internal("ERRORDATA_STACK_SIZE exceeded")));
-    }
-
-    newedata = &errordata[errordata_stack_depth];
+    newedata = push_errordata();
     memcpy(newedata, edata, sizeof(ErrorData));

     /* Make copies of separately-allocated fields */
diff --git a/src/backend/commands/conversioncmds.c b/src/backend/commands/conversioncmds.c
index f067da2..ab744e4 100644
--- a/src/backend/commands/conversioncmds.c
+++ b/src/backend/commands/conversioncmds.c
@@ -49,6 +49,7 @@ CreateConversionCommand(CreateConversionStmt *stmt)
     const char *to_encoding_name = stmt->to_encoding_name;
     List       *func_name = stmt->func_name;
     static Oid    funcargs[] = {INT4OID, INT4OID, CSTRINGOID, INTERNALOID, INT4OID};
+    char result[1];

     /* Convert list of names to a name and namespace */
     namespaceId = QualifiedNameGetCreationNamespace(stmt->conversion_name,
@@ -96,6 +97,19 @@ CreateConversionCommand(CreateConversionStmt *stmt)
                        NameListToString(func_name));

     /*
+     * Check that the conversion function is suitable for the requested
+     * source and target encodings. We do that by calling the function with
+     * an empty string; the conversion function should throw an error if it
+     * can't perform the requested conversion
+     */
+    OidFunctionCall5(funcoid,
+                     Int32GetDatum(from_encoding),
+                     Int32GetDatum(to_encoding),
+                     CStringGetDatum(""),
+                     CStringGetDatum(result),
+                     Int32GetDatum(0));
+
+    /*
      * All seem ok, go ahead (possible failure would be a duplicate conversion
      * name)
      */