Fix #129: enforce MaxClientJsVarBytes on each browser write#147
Merged
Conversation
A normally rendered non-PathSetter JsVar is never rendered again, so the render-time size check could never fire during an active request. A browser could append one valid slice element per Set frame and grow server state without bound (issue #129). Enforce the cap in the client input path instead. JsVar keeps a running serialized-size estimate seeded at render and updated after each browser write by an upper bound on that write's growth (jq.Set only grows the value by appending a single slice element). When the estimate crosses the cap the true size is confirmed by one marshal of the bound value, which also reclaims any over-count; the request is aborted with ErrJsVarTooLarge on the first write whose confirmed size exceeds the cap. Folding an O(1) upper bound per write and marshaling the whole value only at the boundary keeps an append flood O(n) rather than O(n^2). Programmatic JawsSetPath writes remain trusted and uncapped, and PathSetter values remain exempt. Tests cover the append-flood abort, the PathSetter and cap-disabled exemptions, the server-write exemption, and over-count reclamation. BenchmarkJsVarClientWrite guards against reintroducing per-write full-value marshaling by showing per-write cost is independent of the total value size.
jq.Set grows a slice (SetLen) before assigning the appended element, so an append enlarges the bound value even when the assign reports ErrValueUnchanged (a zero-value append) or fails with a type mismatch (a wrong-typed append). The cap only ran size accounting on a clean mutation, so either append flood grew server state past MaxClientJsVarBytes without aborting the request, reproducing the #129 unbounded-growth signature. Account every client write once the cap applies, regardless of the mutation error; withhold only the broadcast when the mutation did not cleanly change the value. The boundary marshal of Ptr remains the ground-truth confirmation, so a bounded value is reclaimed and never false-aborts while any real growth is caught. Add regression tests for the zero-value and rejected append floods.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #129.
Problem
MaxClientJsVarBytesdid not bound a non-PathSetterJsVarafter its normal initial render. The only cap check lived inJsVar.JawsRender, but a normally rendered JsVar is never rendered again. A browser could therefore append one slice element perSetwrite and keep growing server-side state.Fix
Enforce the cap in the client input path while keeping the common path O(1):
jsonByteswith the exact JSON length produced by the initial render.MaxClientJsVarBytes, usejson.Marshalonce to calculate the exact current length. Reset the approximation to that length when it remains under the cap; otherwise abort the request withErrJsVarTooLarge.ErrJsVarTooLarge, abort the request, and retain the underlying marshal error in the cancellation cause.JsVar.setMuand the caller-provided value lock.This deliberately does not duplicate
encoding/jsonsizing rules. Exact JSON work happens only when the raw-byte approximation looks too high.Programmatic
JawsSetPathwrites remain trusted and uncapped.PathSettervalues remain exempt because they enforce their own path and size policy. The render-time check remains for values already over the cap at initial render.Tests and benchmark
Regression coverage includes:
PathSetter, disabled-cap, and server-write exemptions;BenchmarkJsVarClientWriteremains as the O(1)-per-write regression guard. Six-runbenchstatcomparison against the previous PR head (-benchtime=300ms -count=6) is statistically unchanged:Verified with
go test -race -count=1 ./...,go vet ./...,staticcheck ./...,golangci-lint run ./...,gosec ./...,gofumpt,go build ./..., and Linux/386 test compilation.