firedancer-io / firedancer

Firedancer is Jump Crypto's Solana consensus node implementation.
https://firedancer.io
Other
833 stars 150 forks source link

[util] Improve validation in MAP_(remove) #2353

Open 0x0ece opened 1 month ago

0x0ece commented 1 month ago

map_remove doesn't validate that entry (still) is a valid entry in the map. Removing an entry twice can lead to an underflow of key_cnt and passing an invalid entry can lead to an OOB write.

Current callers seem fine, but I think the quic connection / stream handling code is complex enough that a double free of a connection or stream entry could happen.

I think validating the entry and crashing if it's invalid would be a nice defense-in-depth addition.

static inline void
MAP_(remove)( MAP_T * map,
              MAP_T * entry ) {
  MAP_(private_t) * hdr = MAP_(private_from_slot)( map );

  /* FIXME: CONSIDER VALIDATING KEY_CNT AND/OR ENTRY ISN'T VALID */
  hdr->key_cnt--;

  ulong slot_mask = hdr->slot_mask;
  ulong slot      = MAP_(slot_idx)( map, entry );
kbowers-jump commented 1 month ago

A common use (especially in high performance situations like inner loops handing high bandwidth incoming network traffic ... including QUIC) is a map "query" followed very shortly thereafter by a map "remove" (such that the remove target is in the map and a second superfluous query would be very harmful to network throughput ... at 10G and 25G line rates, packet process times often need to be sub-100 ns to keep up).

In cases where the remove target is not obviously in the map, strongly agreed the user should be checking that the "remove" is safe before calling. Would be fine with adding a helper API along the lines of a "remove_if_present" that is guaranteed to succeed and returns whether or not it needed to do anything (just a static inline wrapper of a query / remove pair). Existing call sites to remove can be changed to use this in cases where it is not obvious the target is already in the map.