Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix parsing of IPC packets with multiple or partial messages #176

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mpoc
Copy link

@mpoc mpoc commented Aug 16, 2023

Observed bug

Sometimes creating subscriptions or sending commands through IPC would result in the promise never resolving, i.e. the message with the expected nonce never getting received.

Reason

The original code to parse the received IPC packets:

RPC/src/transports/ipc.js

Lines 82 to 100 in 9e7de2a

if (working.full === '') {
op = working.op = packet.readInt32LE(0);
const len = packet.readInt32LE(4);
raw = packet.slice(8, len + 8);
} else {
raw = packet.toString();
}
try {
const data = JSON.parse(working.full + raw);
callback({ op, data }); // eslint-disable-line callback-return
working.full = '';
working.op = undefined;
} catch (err) {
working.full += raw;
}
decode(socket, callback);
}

The code assumes that a packet will contain at most one message.

The code will skip the processing of a message if a packet contains:

  1. Two complete messages, or
  2. The tail of one message and the head or entirety of the next message.

Practically speaking, we've only seen scenario 1 happen, but theoretically scenario 2 can happen as well.

Scenario 1: A packet contains two complete messages

The packet buffer gets cropped to only contain the message:

raw = packet.slice(8, len + 8);

The rest of the buffer (which contains the second message) gets discarded. The second message never gets decoded.

Scenario 2: A packet contains the tail of one message and the head or entirety of the next message

working.full already contains a part of the message from the previous packet.

This line assumes that the current packet will contain the tail of the message stored in working.full:

raw = packet.toString();

However, because it contains the beginning or entirety of the next message, the parsing of the message fails on line 91, resulting in working.full containing one full message and at least partially a second message because of the concatenation on line 96:

try {
const data = JSON.parse(working.full + raw);
callback({ op, data }); // eslint-disable-line callback-return
working.full = '';
working.op = undefined;
} catch (err) {
working.full += raw;
}

Because working.full is now malformed, it will also affect the decoding of any subsequent messages, causing the packet data to continually be added to working.full.

Fix

Store the expected length of the current payload (link)

const accumulatedData = {
  payload: Buffer.alloc(0),
  op: undefined,
  expectedLength: 0,
};

Treat all received packets as a stream of bytes

Concatenate all received packets (link)

accumulatedData.payload = Buffer.concat([accumulatedData.payload, packet]);

Read a number of bytes according to the received message length (link)

accumulatedData.op = accumulatedData.payload.readInt32LE(0);
accumulatedData.expectedLength = accumulatedData.payload.readInt32LE(4);
accumulatedData.payload = accumulatedData.payload.subarray(8); // Remove opcode and length

Keep the remainder of a packet for decoding of next message (link)

// Accumulated data has the full payload and possibly the beginning of the next payload
const currentPayload = accumulatedData.payload.subarray(0, accumulatedData.expectedLength);
const nextPayload = accumulatedData.payload.subarray(accumulatedData.expectedLength);

accumulatedData.payload = nextPayload; // Keep remainder for next payload

Keep processing the same packet if there's bytes left over after reading the length of the first complete message (link)

while (accumulatedData.payload.length > 0) {
  if (accumulatedData.expectedLength === 0) {
    // We are at the start of a new payload
    accumulatedData.op = accumulatedData.payload.readInt32LE(0);
    accumulatedData.expectedLength = accumulatedData.payload.readInt32LE(4);
    accumulatedData.payload = accumulatedData.payload.subarray(8); // Remove opcode and length
  }

  if (accumulatedData.payload.length < accumulatedData.expectedLength) {
    // Full payload hasn't been received yet, wait for more data
    break;
  }

  // ...
}

@mpoc mpoc marked this pull request as ready for review August 16, 2023 17:57
Copy link
Member

@SpaceEEC SpaceEEC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a way for me to reproduce your issue?
I couldn't come up with something that yielded me more than one payload in a message. 😅

This and the requested change aside, this looks fine to me.

src/transports/ipc.js Outdated Show resolved Hide resolved
@mpoc
Copy link
Author

mpoc commented Aug 18, 2023

Here's a reproducible example:

const RPC = require('discord-rpc')

;(async () => {
    const client = new RPC.Client({ transport: 'ipc' })
    await client.login({
        clientId: '',
        clientSecret: '',
        redirectUri: '',
        scopes: ['rpc', 'rpc.voice.read', 'rpc.voice.write'],
    })
    client.subscribe('VOICE_SETTINGS_UPDATE')
    client.setVoiceSettings({ input: { volume: 100 } })
    client.setVoiceSettings({ output: { volume: 100 } })
})()

The sixth received packet will always have two payloads.

@mpoc mpoc requested a review from SpaceEEC March 4, 2024 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants