Merged
Conversation
maolson-msft
approved these changes
Nov 27, 2019
guhetier
pushed a commit
that referenced
this pull request
Mar 4, 2026
…back type (#5839) Issue: UndefinedBehaviorSanitizer: undefined-behavior ../../src/ext/msquic/src/platform/tls_openssl.c:1251:17 The TicketLength parameter was declared as uint16_t in the function definition but the CXPLAT_TLS_RECEIVE_TICKET_CALLBACK typedef specifies uint32_t. This mismatch caused an undefined behavior sanitizer error when the function was called through the callback pointer. The following is the stack: ``` /usr/bin/llvm-symbolizer-21: /usr/local/lib/libcurl.so.4: no version information available (required by /usr/bin/llvm-symbolizer-21) ../../src/ext/msquic/src/platform/tls_openssl.c:1251:17: runtime error: call to function QuicConnRecvResumptionTicket through pointer to incorrect function type 'unsigned char (*)(struct QUIC_CONNECTION *, unsigned int, const unsigned char *)' /build/out/msquic-build/../../src/ext/msquic/src/core/connection.c:2113: note: QuicConnRecvResumptionTicket defined here #0 0x780a958641e1 in CxPlatTlsOnClientSessionTicketReceived /build/out/msquic-build/../../src/ext/msquic/src/platform/tls_openssl.c:1251:17 #1 0x780a958b2ae1 in ssl_update_cache (/build/out/msquic/lib/libmsquic.so.2+0x4b2ae1) (BuildId: 881896c37f67e076004a0fa8f1497de54832e212) #2 0x780a958e21fd in tls_process_new_session_ticket (/build/out/msquic/lib/libmsquic.so.2+0x4e21fd) (BuildId: 881896c37f67e076004a0fa8f1497de54832e212) #3 0x780a958dd1d7 in state_machine statem.c #4 0x780a958c9ca0 in ssl3_read_bytes (/build/out/msquic/lib/libmsquic.so.2+0x4c9ca0) (BuildId: 881896c37f67e076004a0fa8f1497de54832e212) #5 0x780a958a4eb5 in ssl3_read_internal s3_lib.c #6 0x780a958afebe in SSL_read (/build/out/msquic/lib/libmsquic.so.2+0x4afebe) (BuildId: 881896c37f67e076004a0fa8f1497de54832e212) #7 0x780a95871fea in CxPlatTlsProcessData /build/out/msquic-build/../../src/ext/msquic/src/platform/tls_openssl.c:3319:9 #8 0x780a957678eb in QuicCryptoProcessData /build/out/msquic-build/../../src/ext/msquic/src/core/crypto.c:1959:9 #9 0x780a95772ca4 in QuicCryptoProcessFrame /build/out/msquic-build/../../src/ext/msquic/src/core/crypto.c:1347:14 #10 0x780a9573620b in QuicConnRecvFrames /build/out/msquic-build/../../src/ext/msquic/src/core/connection.c:4622:17 #11 0x780a957405ce in QuicConnRecvDatagramBatch /build/out/msquic-build/../../src/ext/msquic/src/core/connection.c:5604:20 #12 0x780a95743c88 in QuicConnRecvDatagrams /build/out/msquic-build/../../src/ext/msquic/src/core/connection.c:5848:9 #13 0x780a95745c40 in QuicConnFlushRecv /build/out/msquic-build/../../src/ext/msquic/src/core/connection.c:5958:5 #14 0x780a95759e77 in QuicConnDrainOperations /build/out/msquic-build/../../src/ext/msquic/src/core/connection.c:7880:18 #15 0x780a956cd8de in QuicWorkerProcessConnection /build/out/msquic-build/../../src/ext/msquic/src/core/worker.c:658:9 #16 0x780a956c708a in QuicWorkerLoop /build/out/msquic-build/../../src/ext/msquic/src/core/worker.c:881:9 #17 0x780a95852f36 in CxPlatRunExecutionContexts /build/out/msquic-build/../../src/ext/msquic/src/platform/platform_worker.c:667:18 #18 0x780a95853874 in CxPlatWorkerPoolWorkerPoll /build/out/msquic-build/../../src/ext/msquic/src/platform/platform_worker.c:707:5 #19 0x780a9563917c in MsQuicExecutionPoll /build/out/msquic-build/../../src/ext/msquic/src/core/library.c:2839:23 ... ... SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../../src/ext/msquic/src/platform/tls_openssl.c:1251:17 ``` --------- Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: leikong <11276068+leikong@users.noreply.github.com>
guhetier
pushed a commit
that referenced
this pull request
Mar 4, 2026
## Description Fixes memory leaks in `casted_clog_bytearray` when using stdout logging, caused by C's unspecified argument evaluation order. The `clog` macro expands to: ```c struct clog_param * __head = 0; clog_stdout(__head, fmt, casted_clog_bytearray(data, len, &__head)); ``` `casted_clog_bytearray` updates `__head` via the `&__head` pointer, but if the compiler evaluates `__head` (first argument) *before* calling `casted_clog_bytearray`, `clog_stdout` receives NULL for `head` and never frees the allocated memory. The fix changes `clog_stdout` to accept `struct clog_param **` (double pointer) instead of `struct clog_param *`. The macro now passes `&__head`, and `clog_stdout` dereferences it after function entry (when all arguments are fully evaluated), ensuring it always sees the correct linked list head. Changes: - **`src/inc/quic_trace.h`**: Updated both the extern declaration and stub inline definition of `clog_stdout` to use `struct clog_param **`. Updated the `clog` macro to pass `&__head` instead of `__head`. Added an inline comment explaining why the address-of pattern is necessary (the `CASTED_CLOG_BYTEARRAY` macro in `__VA_ARGS__` hardcodes the `__head` variable name and modifies it during argument evaluation). - **`src/generated/stdout/quic_trace.c`**: Updated `clog_stdout` implementation to accept `struct clog_param ** head_ptr` and dereference it on entry (`struct clog_param * head = *head_ptr;`). ## Testing Build verified with `pwsh ./scripts/build.ps1 -loggingType stdout -DisablePerf -DisableTools -Tls openssl -Clean -SanitizeAddress`. All targets compiled and linked without errors. Existing tests cover this path, and the fix is verified by ASAN no longer reporting leaks from `casted_clog_bytearray` allocations. ## Documentation No documentation impact. <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> ---- *This section details on the original issue you should resolve* <issue_title>ASAN leaks in casted_clog_bytearray with stdout logging</issue_title> <issue_description>### Describe the bug All leaks are allocated in casted_clog_bytearray in [quic_trace.c](https://github.com/microsoft/msquic/blob/ed14762f5d55d938aad1c99cbcb287713a0f1e2f/src/generated/stdout/quic_trace.c#L27). This function allocates byte arrays for formatting trace output and they are supposed to be freed in function [clog_stdout(struct clog_param * head, const char * const format, ...)](https://github.com/microsoft/msquic/blob/ed14762f5d55d938aad1c99cbcb287713a0f1e2f/src/generated/stdout/quic_trace.c#L68) before exting: https://github.com/microsoft/msquic/blob/ed14762f5d55d938aad1c99cbcb287713a0f1e2f/src/generated/stdout/quic_trace.c#L119 The issue is that clog_stdout never gets the __head pointer value updated by casted_clog_bytearray. In the following code example: ``` struct clog_param * __head = 0; clog_stdout(__head, fmt, casted_clog_bytearray(data, len, &__head)); ``` casted_clog_bytearray updates __head via [`*head = param;`](https://github.com/microsoft/msquic/blob/ed14762f5d55d938aad1c99cbcb287713a0f1e2f/src/generated/stdout/quic_trace.c#L61)during argument evaluation and the updated __head is expected to be passed into clog_stdout, but this depends on argument evaluation order. When clog_stdout arguments are evaluated from left to right, __head is evaluated before the casted_clog_bytearray() call, so clog_stdout gets NULL `head' and skipped its cleanup loop — leaking all memory allocated in casted_clog_bytearray. The fix is to pass `&__head` to clog_stdout. Regardless of evaluation order, the value of this double pointer is the same. Inside clog_stdout, the pointer is dereferenced after function entry (when all arguments have been fully evaluated), so it always sees the correct linked list head in clog_stdout, thus will be able to free the memory properly. The leaks stop happening with the attached fix: [fix-clog-stdout-memory-leak.patch](https://github.com/user-attachments/files/25310421/fix-clog-stdout-memory-leak.patch) Sample ASAN reports: ``` Direct leak of 192 byte(s) in 12 object(s) allocated from: microsoft/msquic#0 0x58f5d92cc134 in malloc (/home/leikong/repo/meru-common/out/test/quic_transport.test/quic_transport.test+0x1c3134) (BuildId: 1aab74186928132fd9c6fed25feb4cbe93eed340) #1 0x7058b6a1a139 in CxPlatAlloc /home/leikong/repo/meru-common/ext/msquic/src/platform/platform_posix.c:285:12 #2 0x7058b6a4c435 in casted_clog_bytearray /home/leikong/repo/meru-common/ext/msquic/src/generated/stdout/quic_trace.c:32:9 #3 0x7058b6a407c6 in CxPlatSocketContextRecvComplete /home/leikong/repo/meru-common/ext/msquic/src/platform/datapath_epoll.c:1671:9 #4 0x7058b6a41e5a in CxPlatSocketReceiveCoalesced /home/leikong/repo/meru-common/ext/msquic/src/platform/datapath_epoll.c:1805:9 #5 0x7058b6a44075 in CxPlatSocketReceive /home/leikong/repo/meru-common/ext/msquic/src/platform/datapath_epoll.c:1984:13 #6 0x7058b6a37b6e in CxPlatSocketContextIoEventComplete /home/leikong/repo/meru-common/ext/msquic/src/platform/datapath_epoll.c:2606:17 #7 0x58f5d9bdfb9b in quicpp::execution::poll_loop_async(unsigned int) (.resume) /home/leikong/repo/meru-common/src/prod/quicpp/lib/execution.cpp:177:13 #8 0x58f5d93c5b63 in std::__n4861::coroutine_handle<void>::resume() const /usr/lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/coroutine:135:29 #9 0x58f5d93c56b3 in lnm::detail::scheduler_entry_base::resume(bool) /home/leikong/repo/meru-common/ext/base/src/prod/task_scheduler/lib/../inc/task_scheduler/lnm/detail/scheduler_entry_base.h:81:12 #10 0x58f5d9f51550 in lnm::detail::scheduler_entry<unsigned long, std::pair<unsigned int, bool>, true>::resume(std::pair<unsigned int, bool>&&, bool) /home/leikong/repo/meru-common/ext/base/src/prod/task_scheduler/lib/../inc/task_scheduler/lnm/detail/scheduler_entry.h:93:31 #11 0x58f5d9f4df81 in lnm::detail::worker_thread::dispatch_poll(lnm::detail::scheduler_entry<unsigned long, std::pair<unsigned int, bool>, true>*, unsigned int, bool) /home/leikong/repo/meru-common/ext/base/src/prod/task_scheduler/lib/lnm/worker_thread.cpp:1876:12 #12 0x58f5d9f4d4a4 in lnm::detail::worker_thread::handle_poll_event(epoll_event const&) /home/leikong/repo/meru-common/ext/base/src/prod/task_scheduler/lib/lnm/worker_thread.cpp:1546:9 #13 0x58f5d9f4847a in lnm::detail::worker_thread::dispatch_poll(bool) /home/leikong/repo/meru-common/ext/base/src/prod/task_scheduler/lib/lnm/worker_thread.cpp:1463:9 #14 0x58f5d9f4b41c in lnm::detail::worker_thread::dispatch_poll() /home/leikong/repo/meru-common/ext/base/src/prod/task_scheduler/lib/lnm/worker_t... </details> <!-- START COPILOT CODING AGENT SUFFIX --> - Fixes #5790 <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/microsoft/msquic/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: anrossi <41394064+anrossi@users.noreply.github.com> Co-authored-by: mtfriesen <3517159+mtfriesen@users.noreply.github.com>
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
2 participants
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.
Lots of documentation changes and some fairly minor bug fixes and clean up.