fix(spinel): decode forms without losing keys to the GC

spinelkit's Url.parse_query held a decoded key in an unrooted temporary
while decoding the value; on arm64 long titles were silently dropped.
Decode in C inside the tep patch, and run make test again under
SPINEL_GC_STRESS.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Paul Campbell pushed by claude agent, operated by paul@keymail.dev e2c772e29b64dc83c74711aea315f87f3c2dc3d2 parent 60bb659
4 files changed, +149 −12
  • apps/spinel/Makefile +4 −1
  • apps/spinel/README.md +8 −2
  • apps/spinel/app.rb +6 −6
  • apps/spinel/patches/2-tep-bakeoff.patch +131 −3
diff --git a/apps/spinel/Makefile b/apps/spinel/Makefile
index 56bf18b..990e717 100644
--- a/apps/spinel/Makefile
+++ b/apps/spinel/Makefile
@@ -2,7 +2,7 @@
#
# make build dist/app for this machine (x86-64, dynamic glibc)
# make build-arm64 dist/app-linux-arm64 (static, aarch64)
-# make test test/run.sh against dist/app
+# make test test/run.sh against dist/app, plain and under GC stress
# make deploy ship dist/app-linux-arm64 to Carloku
SHELL := bash
@@ -117,8 +117,11 @@ dist/app-linux-arm64: $(SOURCES) version.rb $(ARM64_DEPS)
file $@ | grep -q 'ARM aarch64.*statically linked'
# ---- the rest ----------------------------------------------------------------
+# The second run collects garbage every few KB, which shakes out values
+# the generated code forgot to root (one such bug is why tep is patched).
test: dist/app
test/run.sh dist/app
+ SPINEL_GC_STRESS=1 test/run.sh dist/app
clean:
rm -rf dist .app.tep.rb version.rb .cache/x86/tmp .cache/arm64/tmp .cache/arm64/work
diff --git a/apps/spinel/README.md b/apps/spinel/README.md
index 980e82b..b78b9b0 100644
--- a/apps/spinel/README.md
+++ b/apps/spinel/README.md
@@ -39,8 +39,14 @@ tep is copied into `.cache/` and patched rather than used as-is.
`--listen 127.0.0.1:<port>` (tep's `-p` binds every interface), a 5-second
read timeout so an idle browser connection can't hold the single worker, the
`303 See Other` reason phrase, and a return-type fix in `Tep::Assets._add`
-that otherwise breaks the C compile. Edit `.cache/x86/tep` and run
-`scripts/regen-patch` to change the second one.
+that otherwise breaks the C compile. It also replaces tep's form decoding.
+spinelkit's `Url.parse_query` keeps a decoded key in an unrooted temporary
+while it decodes the value, so a garbage collection in between loses the
+key. On arm64 that dropped any todo title over a thousand or so
+characters; on x86-64 it only shows under `SPINEL_GC_STRESS=1`. The patch
+decodes in C and keeps every intermediate in a local, and `make test` runs
+a second pass under GC stress to catch anything like it. Edit
+`.cache/x86/tep` and run `scripts/regen-patch` to change the second patch.
## Build, run, test
diff --git a/apps/spinel/app.rb b/apps/spinel/app.rb
index 7919e8f..87c0607 100644
--- a/apps/spinel/app.rb
+++ b/apps/spinel/app.rb
@@ -448,7 +448,7 @@ end
post '/todos' do
if !origin_allowed(request)
- return refuse(response, 403, "cross-origin request refused")
+ return refuse(response, 403, "cross-origin POST refused")
end
list = visitor(request, response)
title = request.params["title"].to_s.strip
@@ -460,7 +460,7 @@ end
post '/todos/toggle-all' do
if !origin_allowed(request)
- return refuse(response, 403, "cross-origin request refused")
+ return refuse(response, 403, "cross-origin POST refused")
end
list = visitor(request, response)
l = TodoList.load(list)
@@ -474,7 +474,7 @@ end
post '/todos/clear-completed' do
if !origin_allowed(request)
- return refuse(response, 403, "cross-origin request refused")
+ return refuse(response, 403, "cross-origin POST refused")
end
list = visitor(request, response)
run_sql("DELETE FROM todos WHERE list = ? AND completed != 0", list, 0)
@@ -483,7 +483,7 @@ end
post '/todos/:id/toggle' do
if !origin_allowed(request)
- return refuse(response, 403, "cross-origin request refused")
+ return refuse(response, 403, "cross-origin POST refused")
end
list = visitor(request, response)
id = path_id(request)
@@ -496,7 +496,7 @@ end
post '/todos/:id/edit' do
if !origin_allowed(request)
- return refuse(response, 403, "cross-origin request refused")
+ return refuse(response, 403, "cross-origin POST refused")
end
list = visitor(request, response)
id = path_id(request)
@@ -514,7 +514,7 @@ end
post '/todos/:id/destroy' do
if !origin_allowed(request)
- return refuse(response, 403, "cross-origin request refused")
+ return refuse(response, 403, "cross-origin POST refused")
end
list = visitor(request, response)
id = path_id(request)
diff --git a/apps/spinel/patches/2-tep-bakeoff.patch b/apps/spinel/patches/2-tep-bakeoff.patch
index d6c95ac..1dd846f 100644
--- a/apps/spinel/patches/2-tep-bakeoff.patch
+++ b/apps/spinel/patches/2-tep-bakeoff.patch
@@ -50,15 +50,61 @@
def self.has?(path)
--- a/tep/net.rb
+++ b/tep/net.rb
-@@ -27,6 +27,8 @@
+@@ -27,6 +27,10 @@
# Unix-domain listener: bind+listen on a filesystem path (see
# sphttp.c). -1 on failure; accept/read/write reuse the same fd paths.
ffi_func :sphttp_listen_unix, [:str], :int
+ # TCP listener on one address (`--listen 127.0.0.1:<port>`).
+ ffi_func :sphttp_listen_host, [:str, :int], :int
++ # Form-rules URL decoding in C (see sphttp.c and Tep.parse_form).
++ ffi_func :sphttp_url_unescape, [:str], :str
ffi_func :sphttp_accept, [:int], :int
# Non-blocking accept variant used by Tep::Server::Scheduled.
# Listen fd must be in non-blocking mode (sphttp_set_nonblock).
+--- a/tep/parser.rb
++++ b/tep/parser.rb
+@@ -33,7 +33,7 @@
+ else
+ req.path = req.raw_path[0, qmark]
+ qstring = req.raw_path[qmark + 1, req.raw_path.length - qmark - 1]
+- req.query = SpinelKit::Url.parse_query(qstring)
++ req.query = Tep.parse_form(qstring)
+ end
+
+ i = 1
+@@ -66,7 +66,8 @@
+ if eq > 0
+ cname = pair[0, eq].strip
+ cvalue = pair[eq + 1, pair.length - eq - 1].strip
+- req.cookies[cname] = SpinelKit::Url.unescape(cvalue)
++ cdecoded = Sock.sphttp_url_unescape(cvalue)
++ req.cookies[cname] = cdecoded
+ end
+ end
+ end
+--- a/tep/request.rb
++++ b/tep/request.rb
+@@ -190,7 +190,7 @@
+ # @raw_body to Content-Length.
+ def parse_form_body
+ if form?
+- SpinelKit::Url.parse_query(@raw_body).each do |k, v|
++ Tep.parse_form(@raw_body).each do |k, v|
+ @params[k] = v
+ end
+ elsif multipart?
+--- a/tep/router.rb
++++ b/tep/router.rb
+@@ -77,7 +77,8 @@
+ pp = pat[i]
+ if pp.length > 0 && pp[0] == ":"
+ name = @r_params[pi]
+- req.params[name] = SpinelKit::Url.unescape(rp[i])
++ pdecoded = Sock.sphttp_url_unescape(rp[i])
++ req.params[name] = pdecoded
+ pi += 1
+ end
+ i += 1
--- a/tep/server.rb
+++ b/tep/server.rb
@@ -8,6 +8,7 @@
@@ -118,7 +164,7 @@
extern int sp_net_accept(int sfd);
extern int sp_net_accept_nb(int sfd);
extern int sp_net_connect(const char *host, int port);
-@@ -147,6 +148,17 @@
+@@ -147,7 +148,61 @@
return fd;
}
@@ -134,11 +180,93 @@
+}
+
int sphttp_listen(int port, int reuseport) { return sp_net_listen(port, reuseport); }
++
++/* URL-decode with form rules ('+' is a space). Decoding byte by byte in
++ * Ruby allocates a string per input byte, and SpinelKit::Url.parse_query
++ * holds the decoded key in an unrooted temporary while it decodes the
++ * value, so a collection in between loses the key (seen on aarch64 with
++ * long form values, and on x86-64 under SPINEL_GC_STRESS). Returns a
++ * static buffer; the FFI copies it into a Ruby string. */
++static char *sphttp_unesc_buf = NULL;
++static size_t sphttp_unesc_cap = 0;
++
++static int sphttp_hexval(int c) {
++ if (c >= '0' && c <= '9') return c - '0';
++ if (c >= 'a' && c <= 'f') return c - 'a' + 10;
++ if (c >= 'A' && c <= 'F') return c - 'A' + 10;
++ return -1;
++}
++
++const char *sphttp_url_unescape(const char *s) {
++ size_t n = strlen(s);
++ if (n + 1 > sphttp_unesc_cap) {
++ char *nb = (char *)realloc(sphttp_unesc_buf, n + 1);
++ if (!nb) return "";
++ sphttp_unesc_buf = nb;
++ sphttp_unesc_cap = n + 1;
++ }
++ size_t o = 0;
++ for (size_t i = 0; i < n; i++) {
++ unsigned char c = (unsigned char)s[i];
++ if (c == '+') {
++ sphttp_unesc_buf[o++] = ' ';
++ } else if (c == '%' && i + 2 < n &&
++ sphttp_hexval((unsigned char)s[i + 1]) >= 0 &&
++ sphttp_hexval((unsigned char)s[i + 2]) >= 0) {
++ sphttp_unesc_buf[o++] = (char)(sphttp_hexval((unsigned char)s[i + 1]) * 16 +
++ sphttp_hexval((unsigned char)s[i + 2]));
++ i += 2;
++ } else {
++ sphttp_unesc_buf[o++] = (char)c;
++ }
++ }
++ sphttp_unesc_buf[o] = '\0';
++ return sphttp_unesc_buf;
++}
int sphttp_accept(int sfd) { return sp_net_accept(sfd); }
int sphttp_accept_nb(int sfd) { return sp_net_accept_nb(sfd); }
+
--- a/tep.rb
+++ b/tep.rb
-@@ -300,6 +300,11 @@
+@@ -166,6 +166,37 @@
+ # narrowing model). Removing it would require a mechanical
+ # `< 0` -> `.nil?` refactor across http.rb / parser.rb / url.rb /
+ # jwt.rb / app.rb. Worth doing eventually; not urgent.
++ # Parse an application/x-www-form-urlencoded string (a body or a query
++ # string). Replaces SpinelKit::Url.parse_query, which can lose keys to
++ # the GC (see sphttp_url_unescape); every intermediate here is a local.
++ def self.parse_form(s)
++ h = Tep.str_hash
++ if s.length == 0
++ return h
++ end
++ pairs = s.split("&")
++ i = 0
++ while i < pairs.length
++ pair = pairs[i]
++ if pair.length > 0
++ k = ""
++ v = ""
++ eq = Tep.str_find(pair, "=", 0)
++ if eq < 0
++ k = Sock.sphttp_url_unescape(pair)
++ else
++ kraw = pair[0, eq]
++ vraw = pair[eq + 1, pair.length - eq - 1]
++ k = Sock.sphttp_url_unescape(kraw)
++ v = Sock.sphttp_url_unescape(vraw)
++ end
++ h[k] = v
++ end
++ i += 1
++ end
++ h
++ end
++
+ def self.str_find(s, needle, start)
+ nlen = needle.length
+ slen = s.length
+@@ -300,6 +331,11 @@
Server.new(APP).run_unix(path, quiet)
end