Skip to content

Commit

Permalink
fix race condition in iOS websocket implementation (#32847)
Browse files Browse the repository at this point in the history
Summary:
The iOS WebSocket implementation has a race condition that causes WebSocket frame payloads to be processed incorrectly.
This can cause errors on RFC6455 compliant WebSocket servers:
- the server sends a ping frame with no payload
- the server sends a text frame with a payload longer than 125 bytes
- the client answers the ping with a pong frame echoing back the contents of the text frame

This is caused by concurrent modification of the current frame contents, that is passed by reference to the handlers. The concurrent modification happens [here](https://github.com/facebook/react-native/blob/main/Libraries/WebSocket/RCTSRWebSocket.m#L1162).

The bug was detected and fixed in the original SocketRocket repository in [this PR](facebookincubator/SocketRocket#371). The relevant part of the fix is applied in this PR.

Resolves #30020.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[iOS] [Fixed] - Fix WebSocket control frames having payloads longer than 125 bytes

Pull Request resolved: #32847

Test Plan:
The bug is not easily and consistently reproduced, being a race condition.
We were able to reproduce it by connecting a react native app to a websocket server that sent ~100 pings per second and ~100 text frames per second. After a couple of seconds, the server receives an invalid pong message, with a payload equal to the payload of the text frame. The following is a node server that can replicate the problem on a react-native app running on iOS.

<details>

```
const { WebSocketServer } = require('ws');

const wss = new WebSocketServer({ port: 8080 });

wss.on('connection', function connection(ws) {
  const pingInterval = setInterval(() => {
    ws.ping();
  }, 10);

  const sendInterval = setInterval(() => {
    const arr = new Array(100);
    for (let i = 0; i < arr.length; i++) {
      arr[i] = Math.random();
    }
    ws.send('message with payload longer than 125 bytes: ' + arr.join(','));
  }, 10);

  ws.on('close', () => {
    clearInterval(pingInterval);
    clearInterval(sendInterval);
  });

  ws.on('error', (err) => {
    console.error(err);
    process.exit(1);
  });
});
```

</details>

Reviewed By: hramos

Differential Revision: D33486828

Pulled By: sota000

fbshipit-source-id: ba52958a584d633813e0d623d29b19999d0c617b
  • Loading branch information
asmeikal authored and facebook-github-bot committed Jan 20, 2022
1 parent 0343e69 commit 86db62b
Showing 1 changed file with 4 additions and 0 deletions.
4 changes: 4 additions & 0 deletions Libraries/WebSocket/RCTSRWebSocket.m
Original file line number Diff line number Diff line change
Expand Up @@ -782,6 +782,10 @@ - (void)_disconnect

- (void)_handleFrameWithData:(NSData *)frameData opCode:(NSInteger)opcode
{
// copy frameData before handling,
// to avoid concurrent updates to the value at the pointer
frameData = [frameData copy];

// Check that the current data is valid UTF8

BOOL isControlFrame = (opcode == RCTSROpCodePing || opcode == RCTSROpCodePong || opcode == RCTSROpCodeConnectionClose);
Expand Down

0 comments on commit 86db62b

Please sign in to comment.