Conversation
|
|
||
| timeout, _ := strconv.Atoi(r.URL.Query().Get("timeout")) | ||
| debug, _ := strconv.Atoi(r.URL.Query().Get("debug")) | ||
| timeout, err := strconv.ParseInt(r.URL.Query().Get("timeout"), 10, 32) |
There was a problem hiding this comment.
this doesn't really make sense. iirc valid values for debug are like 0, 1, or 2 and valid values for timeout are... some reasonable number of seconds. would it make more sense to bounds check? i'm not sure limiting the range to int32 is doing much for us
There was a problem hiding this comment.
also negative numbers make no sense since this is... an enum and a number of seconds so if we're trying to make the parser validate the input why parse it as signed?
There was a problem hiding this comment.
My take is, the report doesn't make sense in the first place: If we parse an in64 > INT32_MAX, we'll end up passing an invalid integer to pprof that will either ignore it or fail. The pprof API takes negative integers (and supposedly fails when given that input).
This is meant to quiet down automated reports with the lowest amount of work while not impacting functionality, security or maintainability.
The automated report is a compliance blocker impacting sales.
Alternatively, we may be able to close the report by explaining why we believe this is not an issue, but we should expect scrutiny doing that. Would you rather we go that route?
There was a problem hiding this comment.
the intent of the flag makes sense - we're missing input validation. it's fair to argue that the library does validation on its own and so this is wasted effort. but if we were compelled to add it it seemed like we might as well actually validate the input. if the goal is just checking a box idc
There was a problem hiding this comment.
Valid values for "debug" depend on the profiler used. What does maintaining validation logic for tool name -> valid debug values get us? I can buy that adding an upper limit for the timeout may somewhat mitigate some DOS attempts if anybody ever got access to the pprof endpoint. However, we'd need to make the limit at least 5 min and that's plenty of time to create about as many goroutines as you want.
This validation would however likely make most sense in https://github.com/livekit/protocol/blob/2504a42c8d96f69d22e49462e9b130222a9143b9/pprof/pprof.go#L35. So we would still need to deal with the casting check.
I can make all parameters to all these functions int64 instead, but this feels about as ridiculous.
There was a problem hiding this comment.
if parsing as int32 silences the warning let's ship it...
# Changelog ## Added - Allow passing `projectID` for URL requests (#429) - Introduce linter configuration shared with egress, fixing a wide range of issues including unused params, deprecated APIs, unexported return types, typos, a `TrimLeft` bug, and a mutex-copy bug in a proto message (#425) - Add segment start to adjusted buffer PTS; simplify event probe (#398) - Log segment events to help diagnose non-~0s segments that could push buffer PTSs outside the segment range (#397) - Observe buffer processing latency and expose it as a Prometheus metric (#392) - Gate excess pre-roll buffered media to reduce end-to-end latency (#386) - Ingress metrics (#383) - Pass WHIP through to the SFU directly, with a config option to choose between SFU and native paths; includes WHIP HTTP header handling and RTC closing notify support (#372) - `IngressID` and `ResourceID` attributes on ingress participants (#344) - Custom `HandlerLogger` implementation (#339) - Media watchdog at the tail of the pipeline (input of the Go SDK) to close the ingress if no media is received on any track for 1 minute — needed for cases like SRT where GStreamer may retry connecting forever without emitting any failure event (#334) - Backpressure-aware synchronizer: monitor a queue between the appsink and the Go SDK; if it grows past 2 buffers, reduce the synchronizer wait time in 10ms steps until the queue shortens. Addresses a deadlock case in SRT where the reference/wait time was only ever increased, causing the input buffer to fill to its max. Disabled for HLS, where back pressure at the output is expected. (#337) - Announce out-of-network splice events as participant attributes (derived from SCTE-35 tables in MPEG-TS streams, e.g. SRT). Implemented by monitoring SCTE-35-related GStreamer events pushed down the pipeline — message-bus-based events lack pipeline-timebase timing and the MPEG-TS demuxer doesn't expose the needed info to regenerate timestamps. Relies on a forked `go-gst` for now. (#326) - Reject ingress if the `Enabled` flag is false (#319) ## Changed - Log caps existence when connecting to caps notifier (#431) - Replace deprecated `io/ioutil.ReadFile` with `os.ReadFile` for config loading (#424) - Refactor ingress handler RPC server: remove PSRPC support from the ingress handler and move the full PSRPC server implementation into the server process; introduce a `StateNotifier` family of objects for injecting state-update behavior; add an (initially empty) `ProjectID` field to `StateNotifier` calls (#413) - Only report packet loss if `trackStats` is set (#401) - Update GStreamer to 1.26.7 (#396) - Update Go to 1.25 (#388) - Use `FeatureFlags` from `GetIngressInfoResponse` or `StartIngressRequest`; rename the `SFUTranscodingBypassedWHIP` config option to `WHIPProxyEnabled` (#382) - Fix the format of the logging field in the sample config (#377) - Delay deregistering the WHIP RPC handler to avoid SFU notify warnings (#373) - Switch to the `livekit/gst-go` fork of `go-gst` (#367) - Update CLI to `urfave/cli/v3` (#364) - Set logging parameters on Pion; ignore Pion ICE candidate warning (#348) - Throttle "too slow" logs (#340) - Disable output queue-length monitoring for RTMP and WHIP (#338) - Disable max-buffer limit on URL input queue; log SRT stats every minute (#335) - Initialize URL ingress state with `BUFFERING` when created directly by the Ingress server (#333) - Use logger utilities in ingress (#332) - Remove `actions/cache` from `workflows/build.yaml` — the cloud-ingress build uses Docker, so caching Go modules from the host is pointless (#325) ## Fixed - Fix `int` cast flagged by Copilot (#416) - Ensure the logger is initialized before the output registers for EOS, preventing a race that could cause a panic if EOS arrived during creation (#415) - Ignore all errors from `writeSample` once the output is already closed — not just EOF — so hitting the shutdown timeout no longer sets `pipelineError` and flips the pipeline to a failed state (#412) - Do not treat `io.EOF` as a pipeline error during shutdown; `handleSample()` keeps returning `FlowOK` while samples are dropped so GStreamer can drain its queues without interpreting early `FlowEOS` as a mid-stream failure (#411) - Make sure EOS reaches sinks: signal "EOS seen on source" out-of-band so the output can decide to wait briefly for remaining data (or cancel), preventing the pipeline from freezing when the appsink thread is stuck on a blocking push (#408) - Safer fallback logic for latency reduction: if A/V arrival rates don't stabilize, skip applying offsets entirely instead of taking the current max of calculated offsets (#406) - Initialize logger before the handler starts (#399) - Fix superfluous `response.WriteHeader` call: only send a status code if data hasn't already been written (an implicit 200) (#395) - Make sure an ingress session is terminated when sending on the app source doesn't cause the pipeline to emit an EOS event (#379) - Propagate sink errors when the input error is (generic, consequential) `context.Canceled` (#381) - Fix "disonnected" → "disconnected" typo in error messages (#360) - Fix flaky/broken tests (#378, #374) ## Security - Bump `golang.org/x/image` to v0.38.0 (#426) - Bump `golang.org/x/net` from 0.35.0 to 0.38.0 (#358) - Bump `golang.org/x/crypto` from 0.32.0 to 0.35.0 (#355) - Bump `golang.org/x/net` from 0.31.0 to 0.33.0 (#321)
2 participants
No description provided.