Skip to content

WebSocket OnMessage/OnBinary/OnError never fire: FeedBytes casts an interface back to a class #547

Description

@freitasjca

THorseWebSocketParser.FeedBytes receives the connection as an interface and
hard-casts it back to the class. An interface reference does not point at the
object, so every field read through that cast lands at the wrong address — and
none of the inbound WebSocket callbacks ever fire.

The effect is that a WebSocket server can send but never receive, and it
fails silently.

Observed against an HTTP/2 provider. The defect itself is in
Horse.Core.WebSocket and the cast is unconditional; Indy
(Horse.Provider.Indy.WebSocket.pas:186) and the shared socket transport used
by epoll and IOCP (Horse.Provider.Socket.WebSocket.pas:222) both reach it
through the same HandleIncomingBytes call, so we would expect them to behave
identically — though we have not run them ourselves, and it is worth a quick
check on one of the bundled providers before deciding severity.

Line references are against master at ff41415.

The defect

Horse.Core.WebSocket.pas:812 — the call site converts the class to an interface:

procedure THorseWebSocketConnection.HandleIncomingBytes(const ABytes: TBytes; const ALength: Integer);
begin
  if FIsConnected then
    FParser.FeedBytes(ABytes, ALength, Self);      // Self (class) -> interface
end;

Horse.Core.WebSocket.pas:468 — and the parser casts it straight back:

procedure THorseWebSocketParser.FeedBytes(const ABytes: TBytes; ALength: Integer;
  const AConnection: IHorseWebSocketConnection);   // ...received as interface
...
  if Assigned(THorseWebSocketConnection(AConnection).FOnMessage) then   // ...cast back

An interface reference points at the interface's VMT field within the object,
not at the object itself. Casting it back to the class type does no conversion —
it reinterprets that pointer — so every field access and method call through it
reads from the wrong address.

There are 11 such casts in FeedBytes, covering FOnMessage, FOnBinary,
FOnError and SendRawFrame. In practice:

  • OnMessage never fires
  • OnBinary never fires
  • OnError never fires — which is also why this fails silently
  • ping does not get a pong (SendRawFrame, through the same cast)

Reproduction

Server: any provider, a route calling Res.UpgradeToWebSocket, with
AConn.OnMessage assigned in the connect callback. Client: send any text frame.
The callback does not run — no exception, no log.

We instrumented FeedBytes directly. The frame parses correctly and the
callback reads as unassigned in the same pass:

[core] frame opcode=1 fin=True len=5 consumed=11
[core]   FOnMessage assigned=False

The bytes reaching the parser were a well-formed masked text frame:

81 85  32 49 95 A7  5A 2C F9 CB 5D     ("hello", correctly masked)

Two things this rules out:

  • ParseFrame is not at fault — it returns the right opcode, FIN and length,
    as the trace above shows.
  • SetOnMessage is not at fault — it does FOnMessage := ACallback and had
    already run. SendText on that same connection worked, so the object is live
    and interface dispatch reaches it correctly. Only the read-back through the
    cast is wrong.

The test was driven by an independent HTTP/2 client (Python h2), not by our
own Pascal client, so a symmetric bug on our side could not have produced these
results.

Suggested fix

The call site already holds the class, so typing the parameter as the class
removes the cast entirely:

// declaration
procedure FeedBytes(const ABytes: TBytes; ALength: Integer;
  const AConnection: THorseWebSocketConnection);

// body — direct field access, no cast
if Assigned(AConnection.FOnMessage) then
  AConnection.FOnMessage(AConnection, LMsgText);   // class -> interface is valid

Class-to-interface conversion — still needed, because the callbacks take
IHorseWebSocketConnection — is the safe direction, and the compiler performs it
correctly. HandleIncomingBytes needs no change; it already passes Self.

Both types live in the same unit, so the change is contained to
Horse.Core.WebSocket.pas and is roughly a one-line declaration change plus the
11 cast removals.

Happy to open a PR for this if the approach looks right.

Why it likely went unnoticed

Sending works — SendText goes through the interface properly — so a server
that only pushes messages to clients behaves exactly as expected. Only
receiving is broken, and the failure is silent: no exception, no log, and
OnError unreachable through the same defect. A smoke test that connects and
observes server-pushed messages passes.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions