Skip to content

Fallback rate limiter treats ICMP sockets as streams and can split or truncate a message #176

Description

@ctfbruce

Problem

The change in #171 (head 74eb7cb) makes the fallback limiter datagram-aware, but only for sockets whose local address network starts with udp:

// internal/executor/ratelimit/fallback/count.go:101 at 74eb7cb
datagram := local != nil && strings.HasPrefix(local.Network(), "udp")

ICMP sockets are opened with network ip4:icmp (internal/executor/debuglet/wasm/host_functions.go:149 for connect_ip, and net.ListenPacket("ip4:icmp", ...) in internal/executor/debuglet/netpolicy/icmp.go:37). The local address of such a socket is a *net.IPAddr, whose Network() returns ip, so the connection keeps stream behaviour in the fallback limiter:

  • a Write larger than one rate-second of the limit is split into several socket writes, so one ICMP message becomes several messages, of which only the first carries a valid header;
  • a Read reserves at most one rate-second and hands the socket a shortened buffer, so the kernel truncates the message and discards the rest.

Both are the defects #53 fixed for UDP. #171 records this as a known limit.

Expected behaviour

An ICMP message is admitted whole: one reservation for the whole message, exactly one socket call, refund of what the message did not use, and a zero rate refused before the socket is touched, the same as a UDP datagram.

Proposed change

Treat every packet-oriented network as a datagram connection when attaching: udp* and ip* (ip, ip4, ip6, and the ip4:icmp form). Equivalently, treat only tcp* and unix as streams. This is a one-line decision in Attach plus its tests. Applies on top of #171.

Acceptance

  • A connection whose local address network is ip is a datagram connection; a test with a scripted connection of that network asserts one write for an oversized message and a whole read into a buffer larger than one rate-second.
  • The UDP cases of internal/executor/ratelimit/fallback are unchanged and still pass.
  • The limit note about ICMP in the limiter's comments is removed or corrected.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions