nandi/frq · 6214c7e

nandi/frqpublic⑂ Fork 0

⑂ 6214c7e

Ask for the backlog, and survive hanging up

Two bugs on the same connection.

The history: this client never sent CHATHISTORY. freeq re-joins an
authenticated user's channels at registration and leaves the backlog for
the client to ask for, so a signed-in connection came back to rooms
showing new lines and nothing else. Nothing pointed at it because the
only comments about CHATHISTORY are in `rooms` and `reactions`,
describing a replay that had never been ported — the request went out of
`main.cljd`, which is not a file anything was transcribed from.

It goes out at 366, end of NAMES, which is the one moment a room is
known to have fully arrived. Only where there is no conversation yet:
system lines do not count, and getting that wrong made the first version
of this do nothing at all, since joining a room writes "alice joined
#freeq" into the buffer before 366 lands.

The crash: `SIGSEGV: Illegal storage access. (Attempt to read from
nil?)`, on disconnect. `close` ran on the caller's thread and called
`Socket.close()` — SSL_shutdown and SSL_free — on the handle the reader
thread was inside SSL_read on. A use-after-free in `uniRecv`, and it
took the app with it. `shutdown(2)` on the descriptor instead: it frees
nothing and owns nothing, it just makes the blocked read return, and the
reader closes the socket in its own `finally` as the only thread that
may.

Both of these lived where nothing could see them. The reducer's answer
to an incoming line was the largest untested thing in the program,
reachable only through a real socket. `conn.feed` puts a line in as
though a server had sent it and `conn.tryOutbound` reads what went out,
so the conversation is checkable from both ends without one — and `send`
now queues when there is no connection rather than dropping the line,
with `open` draining what was left over, because a line written to
nowhere should still be readable by the thing testing it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

nandi committed 2026-09-19T09:49:15-07:00 Browse files

6214c7e parent: f912073

modified nim/src/frq/conn.nim +37 -3 Viewed

@@ -16,6 +16,7 @@
16	16	## channels. See irc.nim's comment for why ORC makes that the sane choice.
17	17
18	18	import std/net
	19	+import std/posix as p ## for `shutdown(2)`; see `close`
19	20	import frq/[trace]
20	21
21	22	type
@@ -115,18 +116,51 @@ proc open*(cfg: ConnConfig) =
115	116	# the last one's backlog to the new one's callbacks.
116	117	while inbound.tryRecv()[0]: discard
117	118	while events.tryRecv()[0]: discard
	119	+ # Including anything written while there was nowhere to write it: a line
	120	+ # queued before this connection existed was meant for the last one.
	121	+ while outbound.tryRecv()[0]: discard
118	122	running = true
119	123	createThread(reader, readerBody, cfg)
120	124
121	125	proc send*(line: string) =
122		- if running: outbound.send(line)
	126	+ ## Queued whether or not there is a connection.
	127	+ ##
	128	+ ## It used to drop the line when there was none, which made the whole of
	129	+ ## what this client says to a server unobservable from a test — nothing
	130	+ ## could ask "and what did it send?" without opening a socket. `open` drains
	131	+ ## the queue, so a line written while disconnected is never delivered to the
	132	+ ## next connection; it is simply readable in between.
	133	+ outbound.send(line)
	134	+
	135	+proc tryOutbound*(): (bool, string) = outbound.tryRecv()
	136	+ ## What is queued to go out, for a test that has no socket. The writer
	137	+ ## thread takes these when there is one.
	138	+
	139	+proc feed*(line: string) =
	140	+ ## Put a line in as though the server had sent it.
	141	+ ##
	142	+ ## The other half of `tryOutbound`, and the reason both exist: the reducer's
	143	+ ## answer to an incoming line was the largest untested thing in this program
	144	+ ## — reachable only through a real socket — and that is where a missing
	145	+ ## CHATHISTORY request sat unnoticed through the whole port. Together they
	146	+ ## make the conversation checkable from both ends without one.
	147	+ inbound.send(line)
123	148
124	149	proc close*() =
125	150	if not running: return
126	151	running = false
127		- # The reader is blocked in recvLine; closing the socket is what wakes it.
	152	+ # The reader is blocked in recvLine, and waking it is this thread's job —
	153	+ # but closing the socket is NOT. `Socket.close()` on a TLS socket calls
	154	+ # SSL_shutdown and SSL_free, and the reader is inside SSL_read on that very
	155	+ # handle: freeing it here is a use-after-free in `uniRecv`, which arrives as
	156	+ # `SIGSEGV: Illegal storage access. (Attempt to read from nil?)` and takes
	157	+ # the whole app with it. Every disconnect had a chance of it.
	158	+ #
	159	+ # `shutdown(2)` on the descriptor instead. It frees nothing and owns
	160	+ # nothing; it makes the blocked read return, and the reader then closes the
	161	+ # socket in its own `finally` — the one thread that may.
128	162	if not shared.isNil:
129		- try: shared.close() except CatchableError: discard
	163	+ discard p.shutdown(shared.getFd(), SHUT_RDWR)
130	164	outbound.send("")
131	165	joinThread(reader)
132	166	trace("conn", "closed")

  ## channels. See irc.nim's comment for why ORC makes that the sane choice.
  import std/net
++import std/posix as p   ## for `shutdown(2)`; see `close`
  import frq/[trace]
  type
    # the last one's backlog to the new one's callbacks.
    while inbound.tryRecv()[0]: discard
    while events.tryRecv()[0]: discard
++  # Including anything written while there was nowhere to write it: a line
++  # queued before this connection existed was meant for the last one.
++  while outbound.tryRecv()[0]: discard
    running = true
    createThread(reader, readerBody, cfg)
  proc send*(line: string) =
--  if running: outbound.send(line)
++  ## Queued whether or not there is a connection.
++  ##
++  ## It used to drop the line when there was none, which made the whole of
++  ## what this client says to a server unobservable from a test — nothing
++  ## could ask "and what did it send?" without opening a socket. `open` drains
++  ## the queue, so a line written while disconnected is never delivered to the
++  ## next connection; it is simply readable in between.
++  outbound.send(line)
++
++proc tryOutbound*(): (bool, string) = outbound.tryRecv()
++  ## What is queued to go out, for a test that has no socket. The writer
++  ## thread takes these when there is one.
++
++proc feed*(line: string) =
++  ## Put a line in as though the server had sent it.
++  ##
++  ## The other half of `tryOutbound`, and the reason both exist: the reducer's
++  ## answer to an incoming line was the largest untested thing in this program
++  ## — reachable only through a real socket — and that is where a missing
++  ## CHATHISTORY request sat unnoticed through the whole port. Together they
++  ## make the conversation checkable from both ends without one.
++  inbound.send(line)
  proc close*() =
    if not running: return
    running = false
--  # The reader is blocked in recvLine; closing the socket is what wakes it.
++  # The reader is blocked in recvLine, and waking it is this thread's job —
++  # but closing the socket is NOT. `Socket.close()` on a TLS socket calls
++  # SSL_shutdown and SSL_free, and the reader is inside SSL_read on that very
++  # handle: freeing it here is a use-after-free in `uniRecv`, which arrives as
++  # `SIGSEGV: Illegal storage access. (Attempt to read from nil?)` and takes
++  # the whole app with it. Every disconnect had a chance of it.
++  #
++  # `shutdown(2)` on the descriptor instead. It frees nothing and owns
++  # nothing; it makes the blocked read return, and the reader then closes the
++  # socket in its own `finally` — the one thread that may.
    if not shared.isNil:
--    try: shared.close() except CatchableError: discard
++    discard p.shutdown(shared.getFd(), SHUT_RDWR)
    outbound.send("")
    joinThread(reader)
    trace("conn", "closed")

modified nim/src/frq/reducer.nim +24 -1 Viewed

@@ -11,7 +11,7 @@
11	11	## payload: the alternative is a second serialisation to define and version,
12	12	## for arguments that are always one string.
13	13
14		-import std/[json, options, strutils, tables]
	14	+import std/[json, options, sequtils, strutils, tables]
15	15	import std/sets
16	16	import frq/[cells, model, rooms, reactions, trace, ircparse, clock,
17	17	atproto, handshake, textruns, members, msgsig, profile, store]
@@ -35,6 +35,9 @@ proc setError(msg: string) =
35	35	app.error = msg
36	36	app.hasError = true
37	37
	38	+const historyLimit = 100
	39	+ ## How many lines of backlog to ask a room for.
	40	+
38	41	proc rememberRooms(force = false)
39	42	## Declared here because `openRoom` is above it and calls it — the file is
40	43	## ordered by what the reader does, not by what calls what.
@@ -718,6 +721,26 @@ proc drain*() =
718	721	r.users = r.namesAcc
719	722	r.namesAcc.clear()
720	723	app.rooms[room] = r
	724	+ # And the only moment this client knows a room has fully arrived.
	725	+ #
	726	+ # freeq re-joins an authenticated user's channels at registration
	727	+ # and leaves the backlog for the client to ask for, so a room that
	728	+ # reaches here with an empty buffer has no history coming unless we
	729	+ # ask — which is why nothing but new lines ever appeared. It shows
	730	+ # up worst on a signed-in connection, which is the one that gets
	731	+ # re-joined into rooms it never sent a JOIN for.
	732	+ #
	733	+ # Only where there is no conversation yet: the replayed lines come
	734	+ # back as ordinary PRIVMSGs, and asking again for a room that
	735	+ # already has its history is a second copy of it crossing the wire
	736	+ # to be discarded by the marker.
	737	+ #
	738	+ # System lines do not count, and getting that wrong is what made
	739	+ # the first version of this do nothing at all: joining a room puts
	740	+ # "alice joined #freeq" in the buffer before 366 arrives, so a test
	741	+ # for an empty one is a test that never passes.
	742	+ if not r.messages.anyIt(not it.system):
	743	+ send("CHATHISTORY LATEST " & room & " * " & $historyLimit)
721	744
722	745	of "MODE":
723	746	# A channel MODE, for the letters that change how someone is listed.

  ## payload: the alternative is a second serialisation to define and version,
  ## for arguments that are always one string.
--import std/[json, options, strutils, tables]
++import std/[json, options, sequtils, strutils, tables]
  import std/sets
  import frq/[cells, model, rooms, reactions, trace, ircparse, clock,
             atproto, handshake, textruns, members, msgsig, profile, store]
    app.error = msg
    app.hasError = true
++const historyLimit = 100
++  ## How many lines of backlog to ask a room for.
++
  proc rememberRooms(force = false)
    ## Declared here because `openRoom` is above it and calls it — the file is
    ## ordered by what the reader does, not by what calls what.
              r.users = r.namesAcc
              r.namesAcc.clear()
            app.rooms[room] = r
++          # And the only moment this client knows a room has fully arrived.
++          #
++          # freeq re-joins an authenticated user's channels at registration
++          # and leaves the backlog for the client to ask for, so a room that
++          # reaches here with an empty buffer has no history coming unless we
++          # ask — which is why nothing but new lines ever appeared. It shows
++          # up worst on a signed-in connection, which is the one that gets
++          # re-joined into rooms it never sent a JOIN for.
++          #
++          # Only where there is no conversation yet: the replayed lines come
++          # back as ordinary PRIVMSGs, and asking again for a room that
++          # already has its history is a second copy of it crossing the wire
++          # to be discarded by the marker.
++          #
++          # System lines do not count, and getting that wrong is what made
++          # the first version of this do nothing at all: joining a room puts
++          # "alice joined #freeq" in the buffer before 366 arrives, so a test
++          # for an empty one is a test that never passes.
++          if not r.messages.anyIt(not it.system):
++            send("CHATHISTORY LATEST " & room & " * " & $historyLimit)
      of "MODE":
        # A channel MODE, for the letters that change how someone is listed.

added nim/tests/tsession.nim +106 -0 Viewed

new file mode 100644
@@ -0,0 +1,106 @@
	1	+## What this client says back to a server, given what a server says to it.
	2	+##
	3	+## The reducer's answer to an incoming line was the largest untested thing in
	4	+## the program: reachable only through a real socket, so nothing checked it.
	5	+## That is where the missing CHATHISTORY request sat through the whole port —
	6	+## every room came back with no history and the only comment about it was in
	7	+## another module, describing behaviour that had never been ported.
	8	+##
	9	+## `conn.feed` puts a line in as though the server had sent it; `tryOutbound`
	10	+## reads what went out. No socket at either end.
	11	+
	12	+import std/[sequtils, strutils, tables, unittest]
	13	+import frq/[cells, model, reducer, rooms]
	14	+import frq/conn as tr
	15	+
	16	+proc sent(): seq[string] =
	17	+ ## Everything queued to go out, drained.
	18	+ while true:
	19	+ let (ok, line) = tr.tryOutbound()
	20	+ if not ok: break
	21	+ result.add line
	22	+
	23	+proc say(lines: varargs[string]) =
	24	+ for l in lines: tr.feed(l)
	25	+ drain()
	26	+
	27	+proc reset() =
	28	+ app = initState()
	29	+ app.formNick = "alice"
	30	+ discard sent()
	31	+
	32	+proc joined(room: string) =
	33	+ ## The server putting us in a room, which is how every one of them starts —
	34	+ ## a JOIN we sent, or one freeq made for a signed-in account at
	35	+ ## registration. A 366 for a room this client has never heard of is not
	36	+ ## ours and is left alone.
	37	+ say(":alice!a@h JOIN " & room)
	38	+ discard sent()
	39	+
	40	+suite "asking for the backlog":
	41	+ setup: reset()
	42	+
	43	+ test "end of NAMES in an empty room asks for history":
	44	+ joined("#freeq")
	45	+ # freeq re-joins an authenticated user's channels at registration and
	46	+ # leaves the backlog for the client to ask for. A room that arrives this
	47	+ # way has no history coming unless we ask, which is why a signed-in
	48	+ # connection showed new lines and nothing else.
	49	+ say(":server 366 alice #freeq :End of /NAMES list")
	50	+ check app.rooms.hasKey("#freeq")
	51	+ check sent().anyIt(it.startsWith("CHATHISTORY LATEST #freeq * "))
	52	+
	53	+ test "and asks for a hundred lines of it":
	54	+ joined("#freeq")
	55	+ say(":server 366 alice #freeq :End of /NAMES list")
	56	+ check "CHATHISTORY LATEST #freeq * 100" in sent()
	57	+
	58	+ test "a room that already has lines is not asked twice":
	59	+ # The replay comes back as ordinary PRIVMSGs; asking again is a second
	60	+ # copy of the same history crossing the wire to be thrown away.
	61	+ joined("#freeq")
	62	+ say(":bob!b@h PRIVMSG #freeq :already here")
	63	+ discard sent()
	64	+ say(":server 366 alice #freeq :End of /NAMES list")
	65	+ check not sent().anyIt(it.startsWith("CHATHISTORY"))
	66	+
	67	+ test "a room this client was never put in is not asked about":
	68	+ # Not a room of ours: 366 for it arrives before any JOIN, and answering
	69	+ # it would ask a server for the history of somewhere we are not.
	70	+ say(":server 366 alice #nowhere :End of /NAMES list")
	71	+ check not app.rooms.hasKey("#nowhere")
	72	+ check not sent().anyIt(it.startsWith("CHATHISTORY"))
	73	+
	74	+suite "the rest of the conversation":
	75	+ setup: reset()
	76	+
	77	+ test "a PING is answered with its own token":
	78	+ say("PING :abc123")
	79	+ check "PONG :abc123" in sent()
	80	+
	81	+ test "our own JOIN marks the room joined; somebody else's adds a name":
	82	+ say(":alice!a@h JOIN #freeq")
	83	+ check app.rooms["#freeq"].joined
	84	+ say(":bob!b@h JOIN #freeq")
	85	+ check app.rooms["#freeq"].users.hasKey("bob")
	86	+
	87	+ test "NAMES arrives over several lines and lands in one go":
	88	+ joined("#freeq")
	89	+ say(":server 353 alice = #freeq :alice @bob",
	90	+ ":server 353 alice = #freeq :carol")
	91	+ # Still pending: replacing the list per line empties the panel and
	92	+ # refills it a name at a time.
	93	+ check app.rooms["#freeq"].users.len == 0
	94	+ say(":server 366 alice #freeq :End of /NAMES list")
	95	+ check app.rooms["#freeq"].users.len == 3
	96	+
	97	+ test "a replayed line lands in the room it names":
	98	+ joined("#freeq")
	99	+ say(":server 366 alice #freeq :End of /NAMES list",
	100	+ ":bob!b@h PRIVMSG #freeq :an old line")
	101	+ check app.rooms["#freeq"].messages.anyIt(it.text == "an old line")
	102	+
	103	+ test "a message to us is filed under whoever sent it":
	104	+ say(":bob!b@h PRIVMSG alice :a direct word")
	105	+ check app.rooms.hasKey("bob")
	106	+ check app.rooms["bob"].messages[^1].text == "a direct word"

++## What this client says back to a server, given what a server says to it.
++##
++## The reducer's answer to an incoming line was the largest untested thing in
++## the program: reachable only through a real socket, so nothing checked it.
++## That is where the missing CHATHISTORY request sat through the whole port —
++## every room came back with no history and the only comment about it was in
++## another module, describing behaviour that had never been ported.
++##
++## `conn.feed` puts a line in as though the server had sent it; `tryOutbound`
++## reads what went out. No socket at either end.
++
++import std/[sequtils, strutils, tables, unittest]
++import frq/[cells, model, reducer, rooms]
++import frq/conn as tr
++
++proc sent(): seq[string] =
++  ## Everything queued to go out, drained.
++  while true:
++    let (ok, line) = tr.tryOutbound()
++    if not ok: break
++    result.add line
++
++proc say(lines: varargs[string]) =
++  for l in lines: tr.feed(l)
++  drain()
++
++proc reset() =
++  app = initState()
++  app.formNick = "alice"
++  discard sent()
++
++proc joined(room: string) =
++  ## The server putting us in a room, which is how every one of them starts —
++  ## a JOIN we sent, or one freeq made for a signed-in account at
++  ## registration. A 366 for a room this client has never heard of is not
++  ## ours and is left alone.
++  say(":alice!a@h JOIN " & room)
++  discard sent()
++
++suite "asking for the backlog":
++  setup: reset()
++
++  test "end of NAMES in an empty room asks for history":
++    joined("#freeq")
++    # freeq re-joins an authenticated user's channels at registration and
++    # leaves the backlog for the client to ask for. A room that arrives this
++    # way has no history coming unless we ask, which is why a signed-in
++    # connection showed new lines and nothing else.
++    say(":server 366 alice #freeq :End of /NAMES list")
++    check app.rooms.hasKey("#freeq")
++    check sent().anyIt(it.startsWith("CHATHISTORY LATEST #freeq * "))
++
++  test "and asks for a hundred lines of it":
++    joined("#freeq")
++    say(":server 366 alice #freeq :End of /NAMES list")
++    check "CHATHISTORY LATEST #freeq * 100" in sent()
++
++  test "a room that already has lines is not asked twice":
++    # The replay comes back as ordinary PRIVMSGs; asking again is a second
++    # copy of the same history crossing the wire to be thrown away.
++    joined("#freeq")
++    say(":bob!b@h PRIVMSG #freeq :already here")
++    discard sent()
++    say(":server 366 alice #freeq :End of /NAMES list")
++    check not sent().anyIt(it.startsWith("CHATHISTORY"))
++
++  test "a room this client was never put in is not asked about":
++    # Not a room of ours: 366 for it arrives before any JOIN, and answering
++    # it would ask a server for the history of somewhere we are not.
++    say(":server 366 alice #nowhere :End of /NAMES list")
++    check not app.rooms.hasKey("#nowhere")
++    check not sent().anyIt(it.startsWith("CHATHISTORY"))
++
++suite "the rest of the conversation":
++  setup: reset()
++
++  test "a PING is answered with its own token":
++    say("PING :abc123")
++    check "PONG :abc123" in sent()
++
++  test "our own JOIN marks the room joined; somebody else's adds a name":
++    say(":alice!a@h JOIN #freeq")
++    check app.rooms["#freeq"].joined
++    say(":bob!b@h JOIN #freeq")
++    check app.rooms["#freeq"].users.hasKey("bob")
++
++  test "NAMES arrives over several lines and lands in one go":
++    joined("#freeq")
++    say(":server 353 alice = #freeq :alice @bob",
++        ":server 353 alice = #freeq :carol")
++    # Still pending: replacing the list per line empties the panel and
++    # refills it a name at a time.
++    check app.rooms["#freeq"].users.len == 0
++    say(":server 366 alice #freeq :End of /NAMES list")
++    check app.rooms["#freeq"].users.len == 3
++
++  test "a replayed line lands in the room it names":
++    joined("#freeq")
++    say(":server 366 alice #freeq :End of /NAMES list",
++        ":bob!b@h PRIVMSG #freeq :an old line")
++    check app.rooms["#freeq"].messages.anyIt(it.text == "an old line")
++
++  test "a message to us is filed under whoever sent it":
++    say(":bob!b@h PRIVMSG alice :a direct word")
++    check app.rooms.hasKey("bob")
++    check app.rooms["bob"].messages[^1].text == "a direct word"