Fix RTSP SETUP crash, HLS ctx check bypass, log fd and timer bugs; make core classes testable. v8.0.44 - #4750
Merged
Merged
Conversation
The transmux, SRT, and WHIP proxy E2E scripts skipped WebRTC WHEP playback because FFmpeg has no WHEP demuxer. They now play the stream through the proxy's WHEP API with tools/pion-whep, rebuilt when missing or stale, and require both video and audio RTP packets. The transmux and SRT origins advertise CANDIDATE=127.0.0.1, as the WHIP test already did, because the proxy rewrites only the candidate port. integration-tests.md no longer lists WHEP as a placeholder. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
SrsSrtAcceptor reads _srs_config in method bodies, owns a concrete SrsSrtListener, and calls the srs_srt_set_* functions directly, so no option value, option order, or failure path can be tested without a real SRT socket. This adds the unit tests that state the intended behavior, plus the interfaces they need to compile. There is no runtime behavior change: nothing uses the new interfaces yet. - ISrsSrtListener (fd, create_socket, listen) and ISrsSrtOptions, the 14 SRT options the acceptor sets. - ISrsAppFactory::create_srt_listener, a NULL stub in SrsAppFactory. - The remaining get_srto_* getters on ISrsAppConfig, with the config defaults in MockAppConfig. - config_, app_factory_ and srt_options_ members on SrsSrtAcceptor, initialized to NULL and not yet used. - srs_utest_ai28 with six SrtAcceptorTest cases: listener creation through the factory, every option from config set on the listener fd between create_socket and listen, the passphrase and key length, and create, per-option and listen failures. The six new tests fail on assertions because the acceptor still builds a real listener; the other 2459 unit tests pass. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…ceptor. SrsSrtAcceptor now reaches every collaborator through an interface member, so the SrtAcceptorTest cases can drive it with mocks. The calls, their order, the arguments and the error messages are unchanged. - SrsSrtOptions implements ISrsSrtOptions by forwarding to the srs_srt_set_* functions. - SrsSrtListener implements ISrsSrtListener, and SrsAppFactory::create_srt_listener returns it. - The acceptor captures _srs_config and _srs_app_factory in its constructor and owns an SrsSrtOptions. listen() creates the listener through the factory, and set_srt_opt() uses only config_ and srt_options_. - The acceptor leaves the LCOV exclusion except on_srt_client. All 2465 unit tests pass, including the six SrtAcceptorTest cases. The integration suite, srs-bench black-box and regression tests pass. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
SrsHlsStream::on_timer() expires an idle HLS viewer, but it read _srs_config and _srs_stat directly, so a test could not choose the HLS window or observe the disconnect. This is the change that makes it testable, and the behavior is unchanged: production reaches the same config and statistic objects, now through the members the constructor already captured. - on_timer() uses config_ and stat_, and is no longer excluded from coverage. - Two HlsStreamTest cases, with hooks enabled and disabled: an idle session expires, a recently requested one survives, the on_stop hook is called with its URL, and the disconnect names the expired ctx. - MockAppConfigForLiveStreamHooks takes an hls_window_, and MockStatisticForLiveStream records on_disconnect. - SrsLatestVersion is marked deprecated and unused, so it gets no testability work. Both new tests failed on their assertions before the change: the idle session was kept and neither the hook nor the disconnect was called. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
A client may choose the hls_ctx of a new HLS session in the query string. SrsHlsStream::serve_m3u8_ctx() kept that ctx alive even when serve_new_session() refused the viewer, so a retry with the same ctx was served as an existing session, which reads the playlist without the security check or the on_play hook. Any viewer could therefore pass both by asking twice. The refused viewer also stayed in the statistic until its session expired, and then fired an on_stop hook for playback that never started. A refused viewer now keeps no session: it is removed from the statistic, told why it was refused, and its ctx is never recorded, so every retry is checked again from the start. - HookRejectionTest.HlsRejectedViewerCannotRetryWithSameCtx and HlsDeniedViewerCannotRetryWithSameCtx request the playlist twice with the same client-chosen hls_ctx, for a hook rejection and for a security deny, and require the second request to be refused as well. - MockFileReaderFactoryForHlsStream counts playlist reads, so the tests prove the m3u8 is never read for a refused viewer. Both tests failed before the fix: the retry answered 200 with the playlist while the hook and the security check were called only once. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
SrsHttpStaticServer decides every mount from _srs_config, so no test could drive it: initialize() and mount_vhost() were both excluded from coverage and had none. Add srs_utest_ai29 with a config mock and a mux mock covering the disabled vhost, the http disabled vhost, the configured mount and dir, the [vhost] variable, the trailing slash, the default vhost prefix, a refused mount, the root mount, a vhost that owns the root, directives that are not vhosts, and a vhost that cannot be mounted. Declare the seam the tests inject through: an ISrsAppConfig member captured in the constructor, and get_vhost_http_enabled, get_vhost_http_mount and get_vhost_http_dir on ISrsAppConfig, which only SrsConfig had declared. The method bodies still read the global, so 8 of the 11 tests fail on their assertions and 3 pass, because the real config also declines an unknown vhost and has an empty root. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Use the captured ISrsAppConfig member in initialize() and mount_vhost() so the mount decisions come from the injected dependency, not the global. Same calls, same order, same effects: only the path to config changes. The 11 tests added with the seam now pass, so narrow the coverage exclusion to serve_http, the only function of the class still untested. Verified with the complete C++ suite, 2480 tests with the sanitizer, the cross-component matrix, srs-bench blackbox and regression, and the WebRTC, GB28181 and install-target scripts. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
SrsVodStream was the last class in srs_app_http_static with no injectable collaborator: serve_m3u8_ctx() read _srs_config directly, so the vhost resolution could only be driven by mutating the process global, and no test covered it. Add the goal tests first, in srs_utest_ai29: a config mock that records the vhost it is asked to resolve and the vhost the HLS stream is then asked about, covering a vhost the config resolves to another name, as an alias or a wildcard vhost does, and a vhost it does not resolve. Both failed on their assertions, with the injected config never consulted, because the body still read the global. Then capture ISrsAppConfig in the constructor, use the member in serve_m3u8_ctx(), and null it in the destructor. Same call, same order, same effects: only the path to config changes. Both construction sites are in SrsHttpStaticServer, which captures the same global in its own constructor, so the capture happens at the same point in the lifecycle as before. Verified with the complete C++ suite under the sanitizer, 2482 tests with the Dockerfile.test flags and 2486 with the simulator, the nine-script cross-component matrix, srs-bench blackbox fast and slow, the regression suite, and the browser, GB28181, WebRTC RTX and install-target scripts. Every layer passed. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
SrsFileLog::reopen() closed fd_ without resetting it, then could return early when the tank is the console, as could open_log_file() when the log file name is empty. Both paths left fd_ holding a closed but positive descriptor. write_log() opens a new file only when the descriptor is negative, so it kept writing to that one, and the next reopen() closed it again, by which time the number may have been handed to another socket or file. SIGUSR1 is the entry point, so log rotation is what exercises it. Clear the descriptor together with the close, before any branch, so every path out of reopen() leaves the member consistent with what the process holds. Reaching this from a test needed a seam. SrsFileLog cannot capture the config in its constructor, because _srs_log is created before _srs_config exists, so capture it in initialize(), the first point that provably runs after the config global, and unsubscribe through the same member. get_log_tank_file, get_log_file, get_log_level, get_log_level_v2 and get_utc_time move onto ISrsAppConfig, which only SrsConfig had declared. Add srs_utest_ai30 covering the console tank, the missing log file name, and the normal rotation. All three failed on their assertions first. Narrow the coverage exclusion to initialize, log and write_log, the functions still untested. Verified with the complete C++ suite under the sanitizer, 2489 tests with the simulator and 2485 with the Dockerfile.test flags, the nine-script cross-component matrix, the Go proxy tests, srs-bench blackbox fast and slow, the regression suite, and the browser, GB28181, WebRTC RTX and install-target scripts. Also rotated the log of a running server with SIGUSR1 and confirmed the new file is created and written to. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
SrsFastTimer is the shared heartbeat that connections, sources and the circuit breaker subscribe to, but its constructor created the real coroutine and clock through _srs_kernel_factory and read _srs_context, so a test could never hold a quiescent timer, and cycle() could only be entered with a real ST coroutine and a real sleep. The only coverage was a construct-and-delete smoke test; the dispatch loop itself had none. Add the goal tests first, in srs_utest_ai05, with a kernel factory, coroutine, clock and context mock: the coroutine is created from the injected factory with the timer's own label, handler and context id; every subscriber is notified once per round and the clock is asked to sleep the configured interval after them; a handler error is swallowed without ending the round or stopping the timer; and the round follows the subscription list, so a duplicate subscribe notifies once and an unsubscribed handler is not notified. All four failed on their assertions, with the injected factory never consulted, because construction had already built the real collaborators. Then capture ISrsKernelFactory and ISrsContext in the constructor, keep the label as a member, and move both create calls into assemble(). Same calls, same order, same arguments: only the moment changes, from construction to assemble(), which SrsSharedTimer::initialize() calls at the only four construction sites, before it starts each timer as it did before. Verified with the complete C++ suite under the sanitizer, 2489 tests with the Dockerfile.test flags and 2493 with the simulator, the nine-script cross-component matrix, srs-bench blackbox fast and slow, the regression suite, and the browser, GB28181, WebRTC RTX and install-target scripts. Every layer passed. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…. v8.0.44 SrsFastTimer::cycle() walked handlers_ by index while calling on_timer(), and unsubscribe() erased from that same vector. A handler that left during the round, by unsubscribing itself or from another coroutine that runs while a callback yields on I/O, moved every handler after it one slot back, so the index jumped over one of them and it was not notified that round. Every shared timer carries many subscribers, because the RTCP, TWCC and NACK timers are one per RTC connection and the RTC sources one per stream, so one connection could lose a single 20ms NACK, 100ms TWCC or 1s RTCP round while another connection was torn down. It never crashed, and the next round recovered. Add the tests first, in srs_utest_ai05: a handler that unsubscribes itself must not cost the next handler its notification, a handler unsubscribed ahead of the walk must not be notified after it left, and a handler freed by another handler during the round must not be reached at all. The first failed, with the second of three subscribers notified zero times; the other two passed already, because the erase happened to shift those handlers out of reach, and they hold the round to that behavior. Then notify from a queue instead of walking the vector. Each round copies the handlers into pending_ and pops them off the front, and unsubscribe() removes the handler from pending_ as well, so nothing the round depends on shifts and a handler that left is never reached. Walking a copy taken before the round is not enough: the freed-handler test reads that object and the sanitizer build reports a heap-use-after-free. One behavior change: a handler that subscribes during a round is first notified on the next round, where the index walk notified it in the same round. Verified with the complete C++ suite under the sanitizer, 2496 tests, the Go proxy unit tests, the nine-script cross-component matrix, srs-bench blackbox fast and slow, the regression suite, and the WebRTC RTX, plain retransmission and GB28181 scripts. Every layer passed, with no sanitizer report. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
SrsHourGlass is the tick dispatcher behind the RTMP, RTC, SRT and RTSP source managers and the server's own housekeeping, but its constructor created the real coroutine and clock through _srs_kernel_factory and read _srs_context, so a test could never hold a quiescent hourglass, and cycle() could only be entered with a real ST coroutine and a real sleep. The only coverage was tick and untick; the dispatch loop itself had none. Add the goal tests first, in srs_utest_ai05, reusing the kernel factory, coroutine, clock and context mocks the fast timer already has, plus an error field on MockSrsHourGlass: the coroutine is created from the injected factory as timer-<label> with the hourglass as handler and the context id; a round notifies every event whose interval divides the elapsed time, tells the handler its own interval and the total elapsed time, and sleeps the resolution whether or not an event was due; and a handler error ends the round and stops the timer, unlike the shared fast timer, which swallows it. All three failed on their assertions, with the injected factory never consulted, because construction had already built the real collaborators. Then capture ISrsKernelFactory and ISrsContext in the constructor and move both create calls into assemble(). Same calls, same order, same arguments: only the moment changes, from construction to assemble(), which every construction site calls immediately after new - SrsAppFactory::create_hourglass, as create_origin_hub already did, and the four source managers. Verified with the complete C++ suite under the sanitizer, 2495 tests with the Dockerfile.test flags and 2499 with the preferred flags plus the simulator, the nine-script cross-component matrix, srs-bench blackbox fast and slow, the regression suite, and the browser, GB28181, WebRTC RTX and install-target scripts. Every layer passed. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
SrsRtmpConn is the primary publish and play path, but its constructor changed the process context id, read the cached clock, created the real ST coroutine, called transport_->io() five times to wire the Kbps and delta objects and to build the RTMP server. Only config_->subscribe() had been moved to assemble(). A test could therefore never hold a quiescent connection: by the time it could replace a collaborator, the real one had already been built and used. Add the goal test first, in srs_utest_ai15, with a context and application factory mock beside the coroutine, config and transport mocks the file already has, plus an io_count_ recorder on MockRtmpTransportForDoCycle: construction leaves trd_ and rtmp_ null, create_time_ zero and the transport untouched; then assemble() takes the identity from the injected context, creates the coroutine from the injected factory as "rtmp" with the connection as handler and that context id, wires the transport, builds the RTMP server and subscribes to the injected config. It failed on eleven assertions, with the injected context and factory never consulted, because construction had already done all of it. Then capture ISrsContext next to the application factory and move the five calls into assemble(). Same calls, same order, same arguments: only the moment changes, from construction to assemble(), which both production sites in SrsServer::fd_to_resource already called immediately after new, inside the same SrsContextRestore scope that covered the constructor before. The destructor guards trd_, so a test may hand back a borrowed coroutine. Ten utest construction sites that did not call assemble() now call it, after injecting their dependencies so the subscription and the unsubscription land on the same config. With context_ now a member, the four remaining _srs_context dereferences in method bodies use it: both stat_->on_client() calls and the context ids given to SrsQueueRecvThread and SrsPublishRecvThread. No behavior-affecting global is dereferenced outside the constructor any more. Fix one defect found while reading do_playing(): the no-message branch called srs_usleep(mw_sleep), which does not exist, as the member is mw_sleep_. It survived because SRS_PERF_QUEUE_COND_WAIT is defined unconditionally in srs_core_performance.hpp, so the preprocessor deleted the branch before the compiler saw it. Verified by compiling the file with the branch forced on, which fails before the rename and compiles after. Note this does not make the macro switchable: SRS_PERF_MW_MIN_MSGS and its siblings are nested in the same ifdef and used unconditionally elsewhere. Verified with the complete C++ suite under the sanitizer, 2496 tests with the Dockerfile.test flags and 2500 with the preferred flags plus the simulator, the nine-script cross-component matrix, srs-bench blackbox fast and slow, the regression suite, and the browser, GB28181, WebRTC RTX, plain retransmission and install-target scripts. Every layer passed. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
SrsHttpServer owns the two servers behind one HTTP port, but its constructor created SrsHttpStreamServer and immediately called assemble() on it, which registers that server as a dynamic matcher on its own mux. An owner, or a test, could therefore never put its own stream server in place: by the time it could replace the member, the constructor had already assembled the one it threw away. Nothing else in the class needs a socket, a file or a config, so that single constructor call was the only thing keeping it out of reach. Add the goal test first, in the new srs_utest_ai31, with mock stream and static servers over their existing interfaces and a mock mux that records what it was asked to mount, serve and look up: construction leaves the injected stream server unassembled, and assemble() is what assembles it. That test was red, with assemble_count_ 0 against the expected 1. Then move the call out of the constructor into SrsHttpServer::assemble(), and call it from the one production construction site, SrsServer's constructor, immediately after the new. The static server is now allocated before the stream server is assembled rather than after, which nothing can observe: SrsHttpStaticServer's constructor only allocates its mux and captures the config, and SrsHttpStreamServer::assemble() only touches its own mux. With the seam in place the rest of the class is reachable, so the file also covers the versions API mount, the initialization order and each of its three failures, handle(), http_mount and http_unmount, and the request routing that a coverage exclusion had stood in for until now: /api/ and /console/ to the static server, the "/api" length boundary that is not the API prefix, a mounted stream to the stream server, the fallback when no stream matches, and the failed lookup that must not fall back. The LCOV_EXCL_START/STOP bracket around handle() and serve_http() is removed with it. Verified with the complete C++ suite under the sanitizer, 2510 tests with the Dockerfile.test flags and 2514 with the preferred flags plus the simulator, the nine-script cross-component matrix, srs-bench blackbox fast and slow, the regression suite, and the browser, GB28181, WebRTC RTX, plain retransmission and install-target scripts. Every layer passed. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
SrsRtspConnection is the entry point of every RTSP session, but its constructor
generated the process context id, installed it, and created the real ST
coroutine. Only rtsp_manager_->subscribe() had been moved to assemble(). A test
could therefore never hold a quiescent session: by the time it could replace a
collaborator, the identity and the coroutine already existed.
Add the goal test first, in srs_utest_ai22, with a resource manager mock that
records the dispose subscription beside the generic context, coroutine and
application factory recorders the file already reaches through srs_utest_ai15:
construction leaves trd_ null; then assemble() takes the identity from the
injected context, creates the coroutine from the injected factory as "rtsp" with
the connection as handler and that context id, and subscribes to the injected
manager, whose destructor unsubscribes from the same instance. It failed on
eight assertions, with the injected context and factory never consulted and cid_
already carrying a real generated id, because construction had already done it.
Then capture ISrsAppFactory and ISrsContext beside the five collaborators the
class already held, and move the three calls into assemble(). Same calls, same
order, same arguments: only the moment changes, from construction to assemble(),
which the one production site in SrsServer::fd_to_resource already called
immediately after new. SrsAppFactory::create_coroutine resolves to the same
new SrsSTCoroutine(name, handler, cid) the constructor built by hand. The
destructor guards rtsp_manager_, so a test may unsubscribe from a borrowed mock.
Of the ten utest construction sites, only SessionLifecycleAndDisposal needs the
assembled identity and now calls assemble(); the other nine construct a
quiescent object, which is the seam working as intended.
With context_ now a member, the two remaining _srs_context dereferences in
method bodies use it: on_before_dispose() and switch_to_context(). No
behavior-affecting global is dereferenced outside the constructor any more.
Finish SrsRtspPlayStream in the same file with it. Its constructor was already
quiescent and it already held an injectable app_factory_, but start() built a
concrete SrsFastCoroutine directly, so the started-twice guard and the coroutine
start failure were unreachable without a real coroutine. Two more goal tests
went red first, and the red run also faulted under the sanitizer when that real
coroutine ran cycle() against an uninitialized request, which is the same
untestability seen from the other side. start() now creates the sender coroutine
through app_factory_->create_coroutine("rtsp_sender", ...), as SrsRtcPlayStream
already does. SrsSTCoroutine only forwards to SrsFastCoroutine, so the
construction is unchanged. The orphaned srs_app_st.hpp include goes with it.
Two defects found while reading this class are recorded in the audit and left
for review rather than fixed here, because they are pre-existing and independent
of this refactor: do_setup() assigns networks_[ssrc] without freeing the
previous writer while on_rtsp_request accepts a repeated SETUP with no guard, so
a client that re-SETUPs leaks one SrsRtspTcpNetwork per repeat; and
do_send_packet() reads networks_[ssrc] with std::map::operator[], which
default-inserts a NULL entry on a miss, mutating the map on the send path where
SrsRtspPlayStream::send_packet uses find() correctly.
Verified with the complete C++ suite under the sanitizer, 2517 tests with the
preferred flags plus the simulator, the nine-script cross-component matrix,
srs-bench blackbox fast and slow, the regression suite, and the browser,
GB28181, WebRTC RTX, plain retransmission and install-target scripts. Every
layer passed.
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…SETUP writer. SrsRtspConnection keys tracks_ and networks_ by publisher SSRC and owns every entry in both, but neither was ever erased, and on_rtsp_request is a plain if/else chain with no session state machine. A client may therefore repeat DESCRIBE or SETUP freely, and three defects follow from that. do_describe() stored each track with tracks_.insert(), which keeps the existing entry and drops the new one on the floor. For a live publisher a second DESCRIBE leaked a whole SrsRtcTrackDescription per track and kept answering with the payload type captured by the first one. When the publisher had republished with a new SSRC the insert succeeded instead, leaving the stale entry beside it, and because track_id restarts at 0 on every DESCRIBE both tracks then carried id_ "0". get_ssrc_by_stream_id() scans for the first matching id, so SETUP bound the writer to an SSRC that was no longer published and playback went silent with ERROR_RTSP_NO_TRACK. do_setup() assigned networks_[ssrc] = network without freeing the previous writer, so re-SETUP of a track, which RTSP allows to move it to another interleaved channel, leaked one SrsRtspTcpNetwork per repeat until the connection ended. The SETUP branch swallows do_setup errors and still answers, so repeats are not rate-limited by a teardown. do_send_packet() read networks_[ssrc] with std::map::operator[], which default-inserts a NULL entry on a miss. That allocates a map node on the RTP send path, and the NULL check below it was reading back the entry operator[] had just created. Add the four goal tests first, in srs_utest_ai22, with a stream writer mock that reports its own destruction because macOS has no leak checker. They were red for the intended reasons: the second DESCRIBE still reported payload type 97 instead of 98, proving the new copy was discarded; tracks_ held 2 entries instead of 1 after an SSRC change and get_ssrc_by_stream_id(0) returned the stale 1001 where 1002 was live, proving the stranded-playback path; the replaced writer was never destroyed; and a send-path miss grew networks_ to 1. Then free and clear tracks_ before rebuilding it, placed after the security check, the play hooks and fetch_or_create so a failed DESCRIBE leaves the previous set intact rather than wiping it. This is safe for an active player because SrsRtspSendTrack deep-copies its description in its constructor and initialize() retains no pointer into tracks_, so nothing outside the connection holds those entries. Free the replaced writer in do_setup() before taking the new one. Look up with find() in do_send_packet(), as SrsRtspPlayStream:: send_packet already does a few lines above. One question is left open rather than answered here: on_stream_change() re-keys the track maps to a new publisher SSRC while networks_ stays keyed by the SETUP SSRC, which would strand playback after a publisher reconnect for the same reason as the first defect. It is unverified, needs a black-box publisher restart case, and the method already carries a TODO asking whether it belongs on the RTSP path at all. Verified with the complete C++ suite under the sanitizer, 2521 tests with the preferred flags plus the simulator, the nine-script cross-component matrix, srs-bench blackbox fast and slow, the regression suite, and the browser, GB28181, WebRTC RTX, plain retransmission and install-target scripts. Every layer passed, with 521 Go cases green and no sanitizer report. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
SrsRtspPlayStream's track maps and SrsRtspConnection::networks_ sit on opposite sides of an SSRC rewrite. The track maps are keyed by the publisher SSRC of arriving packets, while SrsRtspSendTrack::on_rtp stamps track_desc_->ssrc_ onto each packet before do_send_packet looks that value up in networks_, which SETUP keyed from the very same track description. on_stream_change() therefore has to re-key the maps to the new publisher SSRC on a republish while leaving track_desc_->ssrc_ alone: moving it would miss in networks_ on every packet from then on, and would also change the SSRC the client negotiated in mid-session. That invariant was the one thing holding the republish path together, and nothing asserted it. OnStreamChange covers the re-keying, the reuse of the track objects and the publisher payload types; OnStreamChangeResetsTrackCache covers the fast cache. Neither looks at the outgoing SSRC. Add OnStreamChangeKeepsSubscriberSsrc, which drives a republish and asserts that the maps follow the new publisher SSRC while each track description keeps the SSRC that SETUP negotiated. It passes as written, because the current behavior is the intended behavior, and its comment says so. Confirmed that it has teeth rather than trusting a green run: assigning the new SSRC to track_desc_ inside on_stream_change kills only this test, at 1002 where 1001 was expected, and leaves OnStreamChange and OnStreamChangeResetsTrackCache green. That one line would have broken RTSP playback after every republish and shipped with the suite passing. The mutation was reverted and srs_app_rtsp_conn.cpp is byte identical to the previous commit; this change is test only. Verified with the complete C++ suite under the sanitizer, 2522 tests with the preferred flags plus the simulator, the nine-script cross-component matrix, srs-bench blackbox fast and slow, the regression suite, and the browser, GB28181, WebRTC RTX, plain retransmission and install-target scripts. Every layer passed, with 521 Go cases green and no sanitizer report. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…serting. The SETUP branch of on_rtsp_request() opened with srs_assert(req->transport_). That pointer is client controlled: SrsRtspRequest::transport_ is initialised to NULL and allocated only when a Transport header is parsed, while is_setup() looks at nothing but the method string. srs_assert is plain assert(), and no SRS build defines NDEBUG, so the assertion is live in release and a failure calls abort(). A SETUP with no Transport header therefore ended the whole process, dropping every session the server was carrying rather than the one connection at fault. Nothing gated the path: the SETUP branch performs no authentication and does not require that a DESCRIBE preceded it, because the security check and the play hooks live in do_describe(), which this never reaches. Any client able to open a TCP connection to the RTSP port could do it, which is the same missing session state machine that lets a client repeat SETUP. Reproduced on a disposable instance before the change: a single such request killed the server, leaving "Assertion failed: (expression), function srs_assert, file srs_kernel_error.cpp, line 502." in its log. Add the goal test first, in srs_utest_ai22. Its red run is the defect itself: the test binary printed the RUN line and then died on that same assertion, with no verdict and no summary. Then refuse the request with 400 Bad Request, which is what RFC 2326 asks for a SETUP missing its required header, and carry on serving the connection. The check has to return early rather than merely drop the assert, because the code below dereferences the transport twice more, in res->transport_->copy() and in the client port fields, so removing the assertion alone would have traded an abort for a null dereference. Verified against a rebuilt server: the same request now answers "RTSP/1.0 400 Bad Request" and the process stays up. This fixes the reachable instance, not the pattern. Three other srs_assert calls sit on paths fed by remote input, at srs_protocol_rtsp_stack.cpp:447 and :715 and at srs_app_rtsp_conn.cpp:1000, and none of them has been audited for reachability with attacker-controlled values. Whether srs_assert belongs on a remote input path at all is left to the maintainer. Verified with the complete C++ suite under the sanitizer, 2523 tests with the preferred flags plus the simulator, the nine-script cross-component matrix, srs-bench blackbox fast and slow, the regression suite, and the browser, GB28181, WebRTC RTX, plain retransmission and install-target scripts. Every layer passed, with 521 Go cases green and no sanitizer report. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…e path is testable. SrsFileLog reached the log file and the console through ::open, ::close, ::write, printf and fflush directly, so log() and write_log() could not be driven by a test and sat inside a coverage exclusion. Both now go through ISrsLogWriter, whose only implementation, SrsLogWriter, holds those calls and nothing else. The logger allocates one in its constructor and reaches every output through the member, so no assemble() is needed and the single production construction site is unchanged. The level-to-color decision stays in write_log() rather than moving into the adapter, which is what makes it assertable. The adapter takes the color code and reproduces the previous bytes exactly, including the plain printf with no reset sequence below the warn level. srs_utest_ai30 now covers the line tail, the color per level, the truncation of an oversized line, the write to a held descriptor, the lazy open when the descriptor is gone, the dropped line when the open fails, the level filter, the disabled level, the formatted message behind its header, and the strerror() an error line appends. The coverage exclusion is left around initialize() alone, which captures _srs_config because the logger is created before that global exists. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
… line. SrsFileLog used fd > 0 as "I hold a log file", but 0 is a valid descriptor and the sentinel for "no file" is -1. A process started with its standard input closed gets 0 from the open() of its log file, and from then on write_log() failed both guards: fd < 0 is false so it never reopened, fd > 0 is false so it never wrote. Every line was discarded for the life of the process while the log file sat there empty. reopen() and the destructor carried the same test, so neither released descriptor 0; SIGUSR1 left the old descriptor open and opened a second one, which accidentally restored logging while leaking the first. The fix is the same comparison in all three places. Reproduced on the real binary with one configuration differing only in standard input: with it open the log file took 48 lines, with 0<&- the file was created and stayed at 0 bytes while the server ran healthily. The rebuilt server writes 41 lines over the same run with standard input closed. Likelihood is low, because systemd and Docker both supply a real standard input, so a supervisor that closes it is what reaches this. The consequence is total silent log loss. srs_utest_ai30 covers the write, the rotation and the destruction of a logger holding descriptor 0. All three were red before the fix. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…injectable members. Every class in srs_kernel_pithy_print reached a process global from a method body, did collaborator work in its constructor, or both, so nothing about the rate limiting could be driven by a test. The existing tests say as much: they call can_print() and assert the answer is either true or false. SrsPithyPrint now captures the stage manager and the clock as ISrsStageManager and ISrsClock and reaches both only through those members. ISrsStageManager is new, declared beside SrsStageManager, which is its only implementation. The clock is the existing ISrsClock, captured from _srs_clock, whose now() returns the srs_time_now_cached() these classes called before, so every value is unchanged. Entering the stage and reading the first tick moved out of the constructor into assemble(). The seventeen factories are the only construction sites, because the constructor is private, so they now share one private create(stage_id) that news and assembles. SrsStageInfo captures ISrsKernelFactory and creates its config from assemble(), which also leaves interval_ initialised where it was previously uninitialised until update_print_time() ran. SrsStageManager assembles the stages it creates. SrsAlonePithyPrint captures the clock and forwards to info_.assemble() from its own assemble(), keeping the original order: the config is read, then the clock. SrsErrorPithyPrint needed no assemble(), because its constructor only assigns; it captures the clock and reaches the three clock reads in can_print() through it. Its stages_ stays a by-value SrsStageManager, which is owned state rather than an injected collaborator. Four construction sites outside the file call assemble() now: SrsGbSession, SrsSrtFrameBuilder, and the two existing tests that build a standalone printer. The manual one is why every site matters: it asserts that a fresh printer cannot print yet, and an unassembled interval of zero lets everything print. srs_utest_ai32 covers what the previous tests could only run without asserting: that assemble() enters the stage through the injected manager, that the constructor enters none, that the destructor leaves the same stage, that elapse() feeds the injected clock delta to both the client age and the shared stage and then caches it, that a backward clock contributes zero, the sharing rule that two clients of one stage print every two intervals, that a stage reads its interval through the injected factory, and that the error printer rate limits each error code separately. Two of the fourteen pass before the change as well, because they exist to prove it altered nothing a caller sees: the stage ids of all seventeen factories, and the interval of a stage a manager hands out. The rest were red first, and the two guarantees the refactor adds were checked by mutation: dropping assemble() from create() kills the factory test with a leftover -1 client count, and dropping the srs_max(0, diff) clamp kills the backward-clock test. Three things found while reading, left unchanged because each would alter behavior rather than the path to a dependency, and recorded in the audit: can_print() treats a stored tick of 0 as "this code has no tick yet", which is the logger's fd > 0 shape and is unreachable only because srs_time_now_cached() never returns 0; update_print_time() has no caller but assemble(), so a reload never changes the interval of a running stage; and create_rtc_send() has no caller outside the tests. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
winlinvip
changed the title
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
1 participant
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.
Bug fixes found while making core classes testable, plus the refactors that made them testable. v8.0.42 to v8.0.44.
Bug fixes
on_rtsp_request()asserted onreq->transport_, which the client controls.srs_assertstays live in release builds, and the path needs no auth and no prior DESCRIBE. So one request from any client that could open a TCP connection to the RTSP port aborted the whole process. The server now answers400 Bad Requestand keeps serving. Confirmed on a real server before and after the fix.hls_ctx.serve_m3u8_ctx()kept the client-chosen ctx even when the security check or theon_playhook refused the viewer. The retry was then served as an existing session and skipped both checks. A refused viewer now keeps no session and is removed from the statistic, so every retry is checked again.reopen()closedfd_but didn't reset it on the console-tank and empty-filename paths. Later writes went to a closed fd whose number may already belong to another socket, and the next reopen closed it again. SIGUSR1 log rotation triggers this. v8.0.43fd > 0as "I have a log file". A process started with stdin closed gets 0 for its log file, and then every line was dropped without any error.SrsFastTimer::cycle()looped over the handlers by index whileunsubscribe()erased from the same vector, so the next handler was skipped. An RTC connection could lose a NACK, TWCC or RTCP round when another connection was torn down. Each round now notifies from a queue. v8.0.44SrsRtspTcpNetworkper repeat. The send path no longer default-inserts NULL entries throughoperator[].srs_usleep(mw_sleep)renamed tomw_sleep_. This was a dead branch that would not compile if enabled.Testability
Collaborators are now injected, and constructor-time work moved into
assemble(), so these classes can be unit tested with mocks:SrsSrtAcceptor: config, listener factory and SRT options behindISrsSrtListener/ISrsSrtOptions.SrsHlsStream::on_timer,SrsHttpStaticServer,SrsVodStream: injected config.SrsFastTimer,SrsHourGlass: the coroutine and clock are created inassemble().SrsRtmpConn,SrsRtspConnection,SrsHttpServer: the context id, coroutine and wiring are set up inassemble().SrsFileLog: output goes throughISrsLogWriter, and config is captured ininitialize().New and extended unit tests are in
srs_utest_ai05,ai15,ai16,ai22,ai25, and the newai28toai32. Several coverage exclusions were narrowed or removed.