Fix rejectCommand trims newline in shared error objects, hung clients (#7714)

65a3307bc (released in 6.0.6) has a side effect, when processCommand
rejects a command with pre-made shared object error string, it trims the
newlines from the end of the string. if that string is later used with
addReply, the newline will be missing, breaking the protocol, and
leaving the client hung.

It seems that the only scenario which this happens is when replying with
-LOADING to some command, and later using that reply from the CONFIG
SET command (still during loading). this will result in hung client.

Refactoring the code in order to avoid trimming these newlines from
shared string objects, and do the newline trimming only in other cases
where it's needed.

Co-authored-by: Guy Benoish <guy.benoish@redislabs.com>

4 files changed, 42 insertions, 23 deletions

diff --git a/src/multi.c b/src/multi.c
index 35ddf92af..a99c308be 100644
--- a/src/multi.c
+++ b/src/multi.c
@@ -127,7 +127,8 @@ void execCommandPropagateExec(client *c) {
 /* Aborts a transaction, with a specific error message.
  * The transaction is always aboarted with -EXECABORT so that the client knows
  * the server exited the multi state, but the actual reason for the abort is
- * included too. */
+ * included too.
+ * Note: 'error' may or may not end with \r\n. see addReplyErrorFormat. */
 void execCommandAbort(client *c, sds error) {
     discardTransaction(c);
 
diff --git a/src/networking.c b/src/networking.c
index 295f75283..4a58cec40 100644
--- a/src/networking.c
+++ b/src/networking.c
@@ -357,14 +357,18 @@ void addReplyProto(client *c, const char *s, size_t len) {
  *
  * If the error code is already passed in the string 's', the error
  * code provided is used, otherwise the string "-ERR " for the generic
- * error code is automatically added. */
+ * error code is automatically added.
+ * Note that 's' must NOT end with \r\n. */
 void addReplyErrorLength(client *c, const char *s, size_t len) {
     /* If the string already starts with "-..." then the error code
      * is provided by the caller. Otherwise we use "-ERR". */
     if (!len || s[0] != '-') addReplyProto(c,"-ERR ",5);
     addReplyProto(c,s,len);
     addReplyProto(c,"\r\n",2);
+}
 
+/* Do some actions after an error reply was sent (Log if needed, updates stats, etc.) */
+void afterErrorReply(client *c, const char *s, size_t len) {
     /* Sometimes it could be normal that a slave replies to a master with
      * an error and this function gets called. Actually the error will never
      * be sent because addReply*() against master clients has no effect...
@@ -390,10 +394,11 @@ void addReplyErrorLength(client *c, const char *s, size_t len) {
             from = "master";
         }
 
+        if (len > 4096) len = 4096;
         char *cmdname = c->lastcmd ? c->lastcmd->name : "<unknown>";
         serverLog(LL_WARNING,"== CRITICAL == This %s is sending an error "
-                             "to its %s: '%s' after processing the command "
-                             "'%s'", from, to, s, cmdname);
+                             "to its %s: '%.*s' after processing the command "
+                             "'%s'", from, to, (int)len, s, cmdname);
         if (ctype == CLIENT_TYPE_MASTER && server.repl_backlog &&
             server.repl_backlog_histlen > 0)
         {
@@ -403,31 +408,39 @@ void addReplyErrorLength(client *c, const char *s, size_t len) {
     }
 }
 
+/* The 'err' object is expected to start with -ERRORCODE and end with \r\n.
+ * Unlike addReplyErrorSds and others alike which rely on addReplyErrorLength. */
+void addReplyErrorObject(client *c, robj *err) {
+    addReply(c, err);
+    afterErrorReply(c, err->ptr, sdslen(err->ptr)-2); /* Ignore trailing \r\n */
+}
+
+/* See addReplyErrorLength for expectations from the input string. */
 void addReplyError(client *c, const char *err) {
     addReplyErrorLength(c,err,strlen(err));
+    afterErrorReply(c,err,strlen(err));
 }
 
-/* See addReplyErrorLength.
- * Makes sure there are no newlines in the string, otherwise invalid protocol
- * is emitted. */
-void addReplyErrorSafe(client *c, char *s, size_t len) {
-    size_t j;
-    /* Trim any newlines at the end (ones will be added by addReplyErrorLength) */
-    while (s[len-1] == '\r' || s[len-1] == '\n')
-        len--;
-    /* Replace any newlines in the rest of the string with spaces. */
-    for (j = 0; j < len; j++) {
-        if (s[j] == '\r' || s[j] == '\n') s[j] = ' ';
-    }
-    addReplyErrorLength(c,s,len);
+/* See addReplyErrorLength for expectations from the input string. */
+void addReplyErrorSds(client *c, sds err) {
+    addReplyErrorLength(c,err,sdslen(err));
+    afterErrorReply(c,err,sdslen(err));
 }
 
+/* See addReplyErrorLength for expectations from the formatted string.
+ * The formatted string is safe to contain \r and \n anywhere. */
 void addReplyErrorFormat(client *c, const char *fmt, ...) {
     va_list ap;
     va_start(ap,fmt);
     sds s = sdscatvprintf(sdsempty(),fmt,ap);
     va_end(ap);
-    addReplyErrorSafe(c, s, sdslen(s));
+    /* Trim any newlines at the end (ones will be added by addReplyErrorLength) */
+    s = sdstrim(s, "\r\n");
+    /* Make sure there are no newlines in the middle of the string, otherwise
+     * invalid protocol is emitted. */
+    s = sdsmapchars(s, "\r\n", "  ",  2);
+    addReplyErrorLength(c,s,sdslen(s));
+    afterErrorReply(c,s,sdslen(s));
     sdsfree(s);
 }
 
diff --git a/src/server.c b/src/server.c
index 6363a9b3b..4da4aeeec 100644
--- a/src/server.c
+++ b/src/server.c
@@ -3498,14 +3498,15 @@ void call(client *c, int flags) {
 /* Used when a command that is ready for execution needs to be rejected, due to
  * varios pre-execution checks. it returns the appropriate error to the client.
  * If there's a transaction is flags it as dirty, and if the command is EXEC,
- * it aborts the transaction. */
+ * it aborts the transaction.
+ * Note: 'reply' is expected to end with \r\n */
 void rejectCommand(client *c, robj *reply) {
     flagTransaction(c);
     if (c->cmd && c->cmd->proc == execCommand) {
         execCommandAbort(c, reply->ptr);
     } else {
         /* using addReplyError* rather than addReply so that the error can be logged. */
-        addReplyErrorSafe(c, reply->ptr, sdslen(reply->ptr));
+        addReplyErrorObject(c, reply);
     }
 }
 
@@ -3515,10 +3516,13 @@ void rejectCommandFormat(client *c, const char *fmt, ...) {
     va_start(ap,fmt);
     sds s = sdscatvprintf(sdsempty(),fmt,ap);
     va_end(ap);
+    /* Make sure there are no newlines in the string, otherwise invalid protocol
+     * is emitted (The args come from the user, they may contain any character). */
+    sdsmapchars(s, "\r\n", "  ",  2);
     if (c->cmd && c->cmd->proc == execCommand) {
         execCommandAbort(c, s);
     } else {
-        addReplyErrorSafe(c, s, sdslen(s));
+        addReplyErrorSds(c, s);
     }
     sdsfree(s);
 }
@@ -3688,7 +3692,7 @@ int processCommand(client *c) {
             rejectCommand(c, shared.bgsaveerr);
         else
             rejectCommandFormat(c,
-                "-MISCONF Errors writing to the AOF file: %s\r\n",
+                "-MISCONF Errors writing to the AOF file: %s",
                 strerror(server.aof_last_write_errno));
         return C_OK;
     }
diff --git a/src/server.h b/src/server.h
index 97f663076..53b4fcce3 100644
--- a/src/server.h
+++ b/src/server.h
@@ -1657,7 +1657,8 @@ void addReplyBulkLongLong(client *c, long long ll);
 void addReply(client *c, robj *obj);
 void addReplySds(client *c, sds s);
 void addReplyBulkSds(client *c, sds s);
-void addReplyErrorSafe(client *c, char *s, size_t len);
+void addReplyErrorObject(client *c, robj *err);
+void addReplyErrorSds(client *c, sds err);
 void addReplyError(client *c, const char *err);
 void addReplyStatus(client *c, const char *status);
 void addReplyDouble(client *c, double d);