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

Code before:

    if (c->peerid == NULL) {
        genClientAddrString(c, peerid, sizeof(peerid), 1);
        c->peerid = sdsnew(peerid);
    }
    return c->peerid;
}

/* This function returns the client bound socket name, by creating and caching
 * it if client->sockname is NULL, otherwise returning the cached value.
 * The Socket Name never changes during the life of the client, however it
 * is expensive to compute. */
char *getClientSockname(client *c) {
    char sockname[NET_ADDR_STR_LEN] = {0};

    if (c->sockname == NULL) {
        genClientAddrString(c, sockname, sizeof(sockname), 0);
        c->sockname = sdsnew(sockname);
    }
    return c->sockname;
}

int isClientConnIpV6(client *c) {
    /* The cached client peer id is on the form "[IPv6]:port" for IPv6
     * addresses, so we just check for '[' here. */
    if ((c->conn == NULL || c->conn->type == NULL) && server.current_client) {
        /* Fake client? Use current client instead. */
        c = server.current_client;
    }
    return getClientPeerId(c)[0] == '[';
}

/* Concatenate a string representing the state of a client in a human
 * readable format, into the sds string 's'. */
sds catClientInfoString(sds s, client *client, int hide_user_data) {
    if (!server.crashed) waitForClientIO(client);
    char flags[17], events[3], conninfo[CONN_INFO_LEN], *p;

    p = flags;
    if (client->flag.replica) {
        if (client->flag.monitor)
            *p++ = 'O';
        else
            *p++ = 'S';
    }

    /* clang-format off */
    if (client->flag.primary) *p++ = 'M';
    if (client->flag.pubsub) *p++ = 'P';
    if (client->flag.multi) *p++ = 'x';
    if (client->flag.blocked) *p++ = 'b';

Review comment (suggestion):
couple of thoughts about this function:

1. This is still not very safe because it assumes `server.current_client` is set and is a real client (btw aof has already violated this in `loadSingleAppendOnlyFile`).
3. I am not sure about the fall back to `current_client` (assuming it is set and real). The caller is asking for `c`'s IPv6 capability not for `current_client`. This implicit conversion can cause surprises in the future IMO.

Overall I feel that this logic is brittle. On a high level, I think we should consider formalizing the concept of `fake` clients, as opposed to implying it from `conn->type` or something else. Then this logic will be as straightforward as ` return !isFakeClient(c) && getClientPeerId(c)[0] == '['`



Code after:
 * if client->peerid is NULL, otherwise returning the cached value.
 * The Peer ID never changes during the life of the client, however it
 * is expensive to compute. */
char *getClientPeerId(client *c) {
    char peerid[NET_ADDR_STR_LEN] = {0};

    if (c->peerid == NULL) {
        genClientAddrString(c, peerid, sizeof(peerid), 1);
        c->peerid = sdsnew(peerid);
    }
    return c->peerid;
}

/* This function returns the client bound socket name, by creating and caching
 * it if client->sockname is NULL, otherwise returning the cached value.
 * The Socket Name never changes during the life of the client, however it
 * is expensive to compute. */
char *getClientSockname(client *c) {
    char sockname[NET_ADDR_STR_LEN] = {0};

    if (c->sockname == NULL) {
        genClientAddrString(c, sockname, sizeof(sockname), 0);
        c->sockname = sdsnew(sockname);
    }
    return c->sockname;
}

int isClientConnIpV6(client *c) {
    /* The cached client peer id is on the form "[IPv6]:port" for IPv6
     * addresses, so we just check for '[' here. */
    if (c->flag.fake && server.current_client) {
        /* Fake client? Use current client instead.
         * Noted that in here we are assuming server.current_client is set
         * and real (aof has already violated this in loadSingleAppendOnlyFil). */
        c = server.current_client;
    }
    return getClientPeerId(c)[0] == '[';
}

/* Concatenate a string representing the state of a client in a human
 * readable format, into the sds string 's'. */
sds catClientInfoString(sds s, client *client, int hide_user_data) {
    if (!server.crashed) waitForClientIO(client);
    char flags[17], events[3], conninfo[CONN_INFO_LEN], *p;

    p = flags;
    if (client->flag.replica) {
        if (client->flag.monitor)
            *p++ = 'O';
        else
            *p++ = 'S';