Repository: valkey-io/valkey
File: src/networking.c (C)

Code before:
     * */
    c->net_input_bytes_curr_cmd = (c->argv_len_sum + (c->argc - 1) + 2);
    c->read_flags |= READ_FLAGS_PARSING_COMPLETED;
}

/* Helper function. Record protocol error details in server log,
 * and set the client as CLIENT_CLOSE_AFTER_REPLY and
 * CLIENT_PROTOCOL_ERROR. */
#define PROTO_DUMP_LEN 128
static void setProtocolError(const char *errstr, client *c) {
    if (server.verbosity <= LL_VERBOSE || c->flag.primary) {
        sds client = catClientInfoString(sdsempty(), c, server.hide_user_data_from_log);
        int loglevel = (c->flag.primary) ? LL_WARNING : LL_VERBOSE;

        if (server.hide_user_data_from_log) {
            serverLog(loglevel, "Query buffer during protocol error: *redacted*");
            goto log_client_and_proto_info;
        }

        /* Sample some protocol to given an idea about what was inside. */
        char buf[256];
        buf[0] = '\0';
        if (c->querybuf && sdslen(c->querybuf) - c->qb_pos < PROTO_DUMP_LEN) {
            snprintf(buf, sizeof(buf), "Query buffer during protocol error: '%s'", c->querybuf + c->qb_pos);
        } else if (c->querybuf) {
            snprintf(buf, sizeof(buf), "Query buffer during protocol error: '%.*s' (... more %zu bytes ...) '%.*s'",
                     PROTO_DUMP_LEN / 2, c->querybuf + c->qb_pos, sdslen(c->querybuf) - c->qb_pos - PROTO_DUMP_LEN,
                     PROTO_DUMP_LEN / 2, c->querybuf + sdslen(c->querybuf) - PROTO_DUMP_LEN / 2);
        }

        /* Remove non printable chars. */
        char *p = buf;
        while (*p != '\0') {
            if (!isprint(*p)) *p = '.';
            p++;
        }

    log_client_and_proto_info:
        /* Log all the client and protocol info. */
        serverLog(loglevel, "Protocol error (%s) from client: %s. %s", errstr, client,
                  server.hide_user_data_from_log ? "*redacted*" : buf);
        sdsfree(client);
    }
    c->flag.close_after_reply = 1;
    c->flag.protocol_error = 1;
}

/* Process the query buffer for client 'c', setting up the client argument
 * vector for command execution.
 * Sets the client's read_flags to indicate the parsing outcome.
 *

Review comment (refactor):
When hide-user-data is disabled, the logged messages look like these, depending on buffer length:

* `"Protocol error (errstr) from client: (client). Query buffer during protocol error: (buf)"`

When hide-user-data is enabled, this code logs

* `"Protocol error (errstr) from client: (client). *redacted*"`
 
The message doesn't say what is redacted. It doesn't say that it's the query buffer.

Another thing that bothers me a little is that "protocol error" appears twice in the logged message. I mean in the old messages. This wasn't well formulated.

To align the two variants and also get rid of the duplicated wording, I think we should remove "Query buffer during protocol error" from the `snprintf` code above (where `buf` is constructed) and add "query buffer" here instead, i.e.

```suggestion
        serverLog(loglevel, "Protocol error (%s) from client: %s. Query buffer: %s", errstr, client,
                  server.hide_user_data_from_log ? "*redacted*" : buf);
```

Code after:
     * */
    c->net_input_bytes_curr_cmd = (c->argv_len_sum + (c->argc - 1) + 2);
    c->read_flags |= READ_FLAGS_PARSING_COMPLETED;
}

/* Helper function. Record protocol error details in server log,
 * and set the client as CLIENT_CLOSE_AFTER_REPLY and
 * CLIENT_PROTOCOL_ERROR. */
#define PROTO_DUMP_LEN 128
static void setProtocolError(const char *errstr, client *c) {
    if (server.verbosity <= LL_VERBOSE || c->flag.primary) {
        sds client = catClientInfoString(sdsempty(), c, server.hide_user_data_from_log);

        /* Sample some protocol to given an idea about what was inside. */
        char buf[256];
        buf[0] = '\0';
        if (server.hide_user_data_from_log) {
            snprintf(buf, sizeof(buf), "*redacted*");
        } else {
            if (c->querybuf && sdslen(c->querybuf) - c->qb_pos < PROTO_DUMP_LEN) {
                snprintf(buf, sizeof(buf), "'%s'", c->querybuf + c->qb_pos);
            } else if (c->querybuf) {
                snprintf(buf, sizeof(buf), "'%.*s' (... more %zu bytes ...) '%.*s'",
                         PROTO_DUMP_LEN / 2, c->querybuf + c->qb_pos, sdslen(c->querybuf) - c->qb_pos - PROTO_DUMP_LEN,
                         PROTO_DUMP_LEN / 2, c->querybuf + sdslen(c->querybuf) - PROTO_DUMP_LEN / 2);
            }

            /* Remove non printable chars. */
            char *p = buf;
            while (*p != '\0') {
                if (!isprint(*p)) *p = '.';
                p++;
            }
        }
        /* Log all the client and protocol info. */
        int loglevel = (c->flag.primary) ? LL_WARNING : LL_VERBOSE;
        serverLog(loglevel, "Protocol error (%s) from client: %s. Query buffer: %s", errstr, client, buf);
        sdsfree(client);
    }
    c->flag.close_after_reply = 1;
    c->flag.protocol_error = 1;
}

/* Process the query buffer for client 'c', setting up the client argument
 * vector for command execution.
 * Sets the client's read_flags to indicate the parsing outcome.
 *
 * This function is called if processInputBuffer() detects that the next
 * command is in RESP format, so the first byte in the command is found
 * to be '*'. Otherwise for inline commands processInlineBuffer() is called. */
void processMultibulkBuffer(client *c) {