1288905 - Questionable reinterpret cast in WebSocketChannel::PrimeNewOutgoingMessage()

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(With error handling omitted)

      uint8_t *buffer;
      mRandomGenerator->GenerateRandomBytes(4, &buffer);
      mask = * reinterpret_cast<uint32_t *>(buffer);

This seems to ignore alignment issues.

Perhaps NativeEndian::readUint32 would work?

Comment 1

[Michal Novotny [:michal]]

Attachment: fix

readUint32() is not public in NativeEndian so I used memcpy

Comment 2

[Patrick McManus [:mcmanus]]

Comment on attachment 8774797 [details] [diff] [review]
fix

Review of attachment 8774797 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +2182,5 @@
>    if (!mIsServerSide) {
>      // Perform the sending mask. Never use a zero mask
>      do {
>        uint8_t *buffer;
> +      nsresult rv = mRandomGenerator->GenerateRandomBytes(sizeof(mask),

thanks for taking this.. as a nit I think you should PR_STATIC_ASSERT (or something - on phone, hard for me to page through alternatives if any) that sizeof mask is 4 as the wirespec clearly needs to deal with 32 bits - just to safely prevent someone from changing the type of mask.

Comment 3

[Michal Novotny [:michal]]

Attachment: patch v2 - added PR_STATIC_ASSERT

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2dba32ae074
Questionable reinterpret cast in WebSocketChannel::PrimeNewOutgoingMessage(), r=mcmanus

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a2dba32ae074