"node:net.Socket": Data Cross-Reception Issue Between Independent Server-Socket Pairs

[DSergiu]

Issue Description

When testing two pairs of independent server-socket connections using the provided code, an unexpected data cross-reception issue occurs. The setup involves creating two separate servers, each with their corresponding client socket. The servers receive data from their respective sockets and echo the data back. However, during the test, the data sent by one socket is received by the other socket. This issue leads to incorrect data reception and potential data corruption.

This issue affects any Deno code using node packages that rely on require(net). Example:. having 2 ioredis.Redis clients connecting to 2 Redis Servers results in issues since one client might receive data from the other client which does not correspond with the actual redis command being sent, thus it results in unexpected errors.

Steps to Reproduce

1. Create a Deno test file named socket.test.ts with the provided test code.
2. Run the test using the command: deno test -A ./socket.test.ts

Expected Behavior

The expected behavior is that each socket should receive the data it sent, without any cross-reception. Specifically:

• Socket1 should send "111-111-111" and receive "111-111-111".
• Socket2 should send "222-222-222" and receive "222-222-222".

Actual Behavior

After running the test, the issue arises where:

• Socket1 receives "222-222-222" instead of the expected "111-111-111".
• Socket2 receives "222-222-222" as expected.

Environment Details

// deno --version
deno 1.36.1 (release, x86_64-unknown-linux-gnu)                            
v8 11.6.189.12                                                             
typescript 5.1.6 

Test Code

// socket.test.ts
import { assertEquals } from "https://deno.land/std@0.198.0/assert/assert_equals.ts";
import net_1 from "node:net";

function setupServerSocketPair(port: number): {
    socket: net_1.Socket,
    res: string[]
} {
    const server = Deno.listen({ port, transport: "tcp" });
    const socket = net_1.createConnection({ port });
    const res: string[] = [];

    (async () => {
        for await (const conn of server) {
            conn.readable.pipeTo(conn.writable);
        }
    })();

    socket.on("data", (data: Uint8Array) => {
        res.push(new TextDecoder().decode(data));
    });

    return {
        socket,
        res
    };
}

Deno.test({
    name: "Multiple Sockets should get correct server data",
    sanitizeResources: false,
    sanitizeOps: false,
}, async () => {
    const setup1 = setupServerSocketPair(10001);
    const setup2 = setupServerSocketPair(20002);

    assertEquals(setup1.socket.write("111-111-111"), true);
    assertEquals(setup2.socket.write("222-222-222"), true);

    // hacky way to wait for socket to receive the data back
    await new Promise((r) => setTimeout(r, 200));

    console.log({ res1: setup1.res, res2: setup2.res });

    assertEquals(setup1.res, [ "111-111-111" ]); // This fails !!!  Actual / Expected "222-222-222"/ "111-111-111"
    assertEquals(setup2.res, [ "222-222-222" ]);
});

[e271828-]

@bartlomieju do you have an ETA for fixing this?

[bartlomieju]

No, not yet. Earliest I'll be able to look into it is next week.

[mmastrac]

@DSergiu thank you greatly for the excellent repro. This was enough to track down the issue. I've turned it into a test case and we'll land a fix ASAP.

[e271828-]

(Redacted until better fix is merged.)

Edit: Now that the fix is released, I should point out that this enabled bugs similar to CVE-2023-28858

@mmastrac it would be appropriate to issue a CVE here. I note Deno is not yet registered as a vendor: https://www.cve.org/PartnerInformation/ListofPartners which would be a good idea.

[DSergiu]

There seems to still be an issue with concurrent reads. Here is a test using npm:ioredis npm package.
Running this test multiple times gives different actual values.

// To run this test:
// 1. Start a redis server instance: "docker run -d --name redis-stack-server -p 6379:6379 redis/redis-stack-server:latest"
// 2. Run test: "deno test -A ./ioredis.test.ts"

import { assertEquals } from "https://deno.land/std@0.198.0/assert/assert_equals.ts";
import { Redis } from "npm:ioredis";

const redisCon = "redis://127.0.0.1:6379";

function buildRedisClient(db: number): Redis {
    const logPrefix = `${redisCon} db=${db}`;
    return new Redis(redisCon, {
        db,
    }).on("error", (err: Error) => {
        console.error(`${logPrefix}: error`, err.message);
    }).on("close", () => {
        console.log(`${logPrefix}: close`);
    }).on("end", () => {
        console.log(`${logPrefix}: end`);
    }).on("connect", () => {
        console.log(`${logPrefix}: connected`, arguments);
    }).on("reconnecting", () => {
        console.log(`${logPrefix}: reconnecting`, arguments);
    }).on("ready", () => {
        console.log(`${logPrefix}: ready`, arguments);
    });
}

Deno.test({
    name: "Concurrent ioredis clients",
    sanitizeResources: false,
    sanitizeOps: false,
}, async (t) => {

    const redis1 = buildRedisClient(1);
    const redis2 = buildRedisClient(2);

    await t.step("it should set and get a value from multiple dbs", async () => {
        const key1 = "key1";
        const key2 = "key2";

        await Promise.all([
            redis1.set(key1, "111"),
            redis2.set(key2, "222"),
        ]);
        const [actual1, actual2, none] = await Promise.all([
            redis1.get(key1),
            redis2.get(key2),
            redis1.get("key3"),
        ]);
        assertEquals(actual1, "111"); // This fails, actual1 is "222"
        assertEquals(actual2, "222");
        assertEquals(none, null);
    });

    await redis1.disconnect();
    await redis2.disconnect();
});

here are some logs for stream #write and #data event:

stream #write { id: 1, cmd: "*2\r\n$3\r\nget\r\n$4\r\nkey1\r\n" }
stream #write { id: 2, cmd: "*2\r\n$3\r\nget\r\n$4\r\nkey2\r\n" }
stream #write { id: 1, cmd: "*2\r\n$3\r\nget\r\n$3\r\nkey3\r\n" }
stream #data { id: 1, data: "$3\r\n222\r\n" }
stream #data { id: 2, data: "$3\r\n222\r\n" }
stream #data { id: 1, data: "$-1\r\n" }

Deno version:

deno 1.36.3 (release, x86_64-unknown-linux-gnu)
v8 11.6.189.12
typescript 5.1.6

[DSergiu]

deno upgrade --canary seems to have the fix. This ticket can stay closed. Thank you for the quick fix @mmastrac .

deno 1.36.3+fb7092f (canary, x86_64-unknown-linux-gnu)
v8 11.6.189.12
typescript 5.1.6