[None][fix] Fix: Using RAII to automatically manage the allocation and release of va_list for potential resource leak #6758
Conversation
📝 WalkthroughWalkthroughAdds an RAII Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant fmtstr
participant fmtstr_
participant vsnprintf
Caller->>fmtstr: call fmtstr(format, ...)
fmtstr->>fmtstr: va_start(args)\nconstruct va_list_guard(args)
fmtstr->>fmtstr_: call fmtstr_(format, alloc, target, args)
fmtstr_->>fmtstr_: va_copy(args0, args)\nvsnprintf(..., args0)\nva_end(args0)
alt need more space
fmtstr_->>vsnprintf: vsnprintf(..., args)
end
fmtstr->>fmtstr: va_list_guard destructor -> va_end(args)
fmtstr->>Caller: return result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
Fan-Yunfan
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
cpp/tensorrt_llm/common/stringUtils.cpp (1)
46-55: Documentfmtstr_allocator’s null-terminator contract
The default allocator (std::string::resize(count)) guarantees storage forcountcharacters plus a trailing'\0', so callers ofalloc(target, n)get at leastn+1bytes. Make this explicit in the hot path to prevent future one-byte overflows:File: cpp/tensorrt_llm/common/stringUtils.cpp
Lines: 46- auto* memory = alloc(target, size); + // alloc(target, size) is required to return a buffer of at least (size + 1) bytes + // so we can safely write size characters plus the null terminator. + auto* memory = alloc(target, size);
🧹 Nitpick comments (1)
cpp/tensorrt_llm/common/stringUtils.cpp (1)
2-2: Nit: Update copyright yearGuidelines require headers to include the current year. Consider updating to 2025.
- * Copyright (c) 2022-2024, NVIDIA CORPORATION. All rights reserved. + * Copyright (c) 2022-2025, NVIDIA CORPORATION. All rights reserved.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cpp/tensorrt_llm/common/stringUtils.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,h,hpp,cc,cxx}
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
**/*.{cpp,h,hpp,cc,cxx}: Closing braces of namespaces should have a comment saying the namespace it closes (e.g., } // namespace foo).
Prefer const or constexpr variables over #defines whenever possible.
A variable that is not modified after its initialization should be declared as const.
Except 0 (used for checking signness/existence/emptiness), nullptr, true, false, all other literals should only be used for variable initialization.
Use the Allman indentation style for braces in C++ code.
Put the semicolon for an empty for or while loop in a new line.
The statement forming the body of a switch, while, do..while, or for statement shall be a compound statement (use brace-delimited statements).
If and else should always be followed by brace-delimited statements, even if empty or a single statement.
C++ filenames should use camel case with the first letter lowercase (e.g., thisIsAFilename.cpp), and all files involved in a compilation target must have case-insensitive unique filenames.
All types (including class names) should use camel case with uppercase first letter (e.g., FooBarClass).
Local variables, methods, and namespaces should use camel case with first letter lowercase (e.g., localFooBar).
Non-magic-number global variables that are non-static and not defined in anonymous namespace should use camel case prefixed by 'g' (e.g., gDontUseGlobalFoos).
Non-magic-number global variables that are static or defined in an anonymous namespace should use camel case prefixed by 's' (e.g., sMutableStaticGlobal).
Locally visible static variables should use camel case with lowercase prefix 's' as the first letter (e.g., static std::once_flag sFlag;).
Class member variables should use camel case prefixed with 'm' (e.g., mNbFooValues). Public member variables do not require the 'm' prefix but it is encouraged for clarity.
Enumerations, global constants, static constants at class-scope, and function-scope magic-number/literal constants should be uppercase snake case with prefix...
Files:
cpp/tensorrt_llm/common/stringUtils.cpp
**/*.{cpp,h,hpp,cc,cxx,cu,py}
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
All TensorRT-LLM Open Source Software code should contain an NVIDIA copyright header that includes the current year. This includes .cpp, .h, .cu, .py, and any other source files which are compiled or interpreted.
Files:
cpp/tensorrt_llm/common/stringUtils.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: fyf2016
PR: NVIDIA/TensorRT-LLM#6679
File: cpp/include/tensorrt_llm/common/logger.h:63-68
Timestamp: 2025-08-07T01:45:23.301Z
Learning: In C++ variadic template functions, the compiler implicitly inserts positional placeholders for template parameter packs during __attribute__((format(printf, ...))) processing, causing an offset in parameter numbering. The format string parameter index should account for this template expansion mechanism rather than the nominal parameter position.
📚 Learning: 2025-08-07T01:45:23.301Z
Learnt from: fyf2016
PR: NVIDIA/TensorRT-LLM#6679
File: cpp/include/tensorrt_llm/common/logger.h:63-68
Timestamp: 2025-08-07T01:45:23.301Z
Learning: In C++ variadic template functions, the compiler implicitly inserts positional placeholders for template parameter packs during __attribute__((format(printf, ...))) processing, causing an offset in parameter numbering. The format string parameter index should account for this template expansion mechanism rather than the nominal parameter position.
Applied to files:
cpp/tensorrt_llm/common/stringUtils.cpp
📚 Learning: 2025-08-07T01:45:23.301Z
Learnt from: fyf2016
PR: NVIDIA/TensorRT-LLM#6679
File: cpp/include/tensorrt_llm/common/logger.h:63-68
Timestamp: 2025-08-07T01:45:23.301Z
Learning: In GCC/Clang, printf format attributes with C++ variadic templates use the same parameter numbering as C-style variadics: parameter 1 is the format string, parameter 2 is where the template parameter pack begins. There are no implicit positional placeholders that offset the numbering. The syntax is __attribute__((format(printf, fmt_idx, args_idx))) where fmt_idx and args_idx are 1-based indices.
Applied to files:
cpp/tensorrt_llm/common/stringUtils.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit Check
🔇 Additional comments (1)
cpp/tensorrt_llm/common/stringUtils.cpp (1)
37-39: Correct: Pair va_copy with va_end to avoid UB/resource leaksGood fix. This correctly matches the va_copy(args0, args) with va_end(args0) and prevents UB on ABIs where va_list requires cleanup. Placement right after the first vsnprintf ensures cleanup even on error paths.
|
Dear @tongyuantongyu ，I noticed that you have made some related modifications to the stringUtils.cpp file (a139eae), so I was wondering if you could take a look at this PR I submitted when you have some free time? 
By the way, I have another PR（https://github.com/NVIDIA/TensorRT-LLM/pull/6723） that needs your review. |
svc-trtllm-gh-bot
added
the
Community want to contribute
|
Thank you for the contribution. Can you fix the issue in Notice that if |
Of course, I'll update the code later tonight to fix this part. Thank you for your review ～ 
( By the way, I have another PR about the const modifier（#6679) that needs your review. |
Fan-Yunfan
force-pushed
the
fyf_fix_va_end
branch
from
0bb3e99 to
af5cc02
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cpp/include/tensorrt_llm/common/stringUtils.h(4 hunks)cpp/tensorrt_llm/common/stringUtils.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,h,hpp,cc,cxx}
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
**/*.{cpp,h,hpp,cc,cxx}: Closing braces of namespaces should have a comment saying the namespace it closes (e.g., } // namespace foo).
Prefer const or constexpr variables over #defines whenever possible.
A variable that is not modified after its initialization should be declared as const.
Except 0 (used for checking signness/existence/emptiness), nullptr, true, false, all other literals should only be used for variable initialization.
Use the Allman indentation style for braces in C++ code.
Put the semicolon for an empty for or while loop in a new line.
The statement forming the body of a switch, while, do..while, or for statement shall be a compound statement (use brace-delimited statements).
If and else should always be followed by brace-delimited statements, even if empty or a single statement.
C++ filenames should use camel case with the first letter lowercase (e.g., thisIsAFilename.cpp), and all files involved in a compilation target must have case-insensitive unique filenames.
All types (including class names) should use camel case with uppercase first letter (e.g., FooBarClass).
Local variables, methods, and namespaces should use camel case with first letter lowercase (e.g., localFooBar).
Non-magic-number global variables that are non-static and not defined in anonymous namespace should use camel case prefixed by 'g' (e.g., gDontUseGlobalFoos).
Non-magic-number global variables that are static or defined in an anonymous namespace should use camel case prefixed by 's' (e.g., sMutableStaticGlobal).
Locally visible static variables should use camel case with lowercase prefix 's' as the first letter (e.g., static std::once_flag sFlag;).
Class member variables should use camel case prefixed with 'm' (e.g., mNbFooValues). Public member variables do not require the 'm' prefix but it is encouraged for clarity.
Enumerations, global constants, static constants at class-scope, and function-scope magic-number/literal constants should be uppercase snake case with prefix...
Files:
cpp/include/tensorrt_llm/common/stringUtils.hcpp/tensorrt_llm/common/stringUtils.cpp
**/*.{h,hpp}
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
Use a preprocessor guard in header files. The guard name must have prefix TRTLLM_ followed by the filename, all in caps, and no trailing underscore.
Files:
cpp/include/tensorrt_llm/common/stringUtils.h
**/*.{cpp,h,hpp,cc,cxx,cu,py}
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
All TensorRT-LLM Open Source Software code should contain an NVIDIA copyright header that includes the current year. This includes .cpp, .h, .cu, .py, and any other source files which are compiled or interpreted.
Files:
cpp/include/tensorrt_llm/common/stringUtils.hcpp/tensorrt_llm/common/stringUtils.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit Check
🔇 Additional comments (5)
cpp/include/tensorrt_llm/common/stringUtils.h (3)
77-109: Excellent RAII implementation for va_list management.The
va_list_wrapperclass properly addresses the resource leak issue by ensuringva_end()is always called via destructor. The design is solid:
- Two constructors handle both
va_startandva_copyscenarios- Non-copyable to prevent double-free issues
- RAII ensures cleanup even with exceptions
get()method provides access to underlyingva_list
139-139: Function signature correctly updated for RAII approach.The change from
va_list argstova_list_wrapper& argsaligns with the RAII design and ensures proper resource management.
151-165: Clean integration of va_list_wrapper in fmtstr().The function now properly constructs the wrapper and passes it by reference to
fmtstr_(). The RAII wrapper eliminates the need for explicitva_end()calls, making the code more exception-safe.cpp/tensorrt_llm/common/stringUtils.cpp (2)
29-29: Function signature correctly updated to match header.The parameter change to
va_list_wrapper& argsis consistent with the header declaration and RAII approach.
35-35: Proper usage of wrapper in first vsnprintf call.Using
args0.get()correctly accesses the underlyingva_listfrom the wrapper.
Fan-Yunfan
force-pushed
the
fyf_fix_va_end
branch
from
af5cc02 to
db9c3cd
Compare
Fan-Yunfan
changed the title
Dear @tongyuantongyu, Thank you for raising the issue regarding Solution:I employed the RAII (Resource Acquisition Is Initialization) mechanism to automatically manage the allocation and deallocation of Key Changes & Design Philosophy:
Let me know if you have any further questions or suggestions! By the way, would you mind reviewing another PR（https://github.com/NVIDIA/TensorRT-LLM/pull/6679）? (This one may not touch areas you've previously worked on) |
… va_list Format: Code style alignment Signed-off-by: fanyunfan <[email protected]>
Fan-Yunfan
force-pushed
the
fyf_fix_va_end
branch
from
db9c3cd to
1e6a6ca
Compare
Signed-off-by: fanyunfan <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cpp/include/tensorrt_llm/common/stringUtils.h (1)
135-135: Consider marking the function parameter as const reference.Since
fmtstr_only needs to read from the va_list (viava_copy), the parameter could be marked asconst va_list_guard&to better express the intent and prevent accidental modifications.-void fmtstr_(char const* format, fmtstr_allocator alloc, void* target, va_list_guard& args_guard); +void fmtstr_(char const* format, fmtstr_allocator alloc, void* target, const va_list_guard& args_guard);Note: If you apply this change, you'll also need to add a const version of the
get()method in theva_list_guardclass:va_list& get() { return args_; } + + const va_list& get() const + { + return args_; + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cpp/include/tensorrt_llm/common/stringUtils.h(4 hunks)cpp/tensorrt_llm/common/stringUtils.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,h,hpp,cc,cxx}
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
**/*.{cpp,h,hpp,cc,cxx}: Closing braces of namespaces should have a comment saying the namespace it closes (e.g., } // namespace foo).
Prefer const or constexpr variables over #defines whenever possible.
A variable that is not modified after its initialization should be declared as const.
Except 0 (used for checking signness/existence/emptiness), nullptr, true, false, all other literals should only be used for variable initialization.
Use the Allman indentation style for braces in C++ code.
Put the semicolon for an empty for or while loop in a new line.
The statement forming the body of a switch, while, do..while, or for statement shall be a compound statement (use brace-delimited statements).
If and else should always be followed by brace-delimited statements, even if empty or a single statement.
C++ filenames should use camel case with the first letter lowercase (e.g., thisIsAFilename.cpp), and all files involved in a compilation target must have case-insensitive unique filenames.
All types (including class names) should use camel case with uppercase first letter (e.g., FooBarClass).
Local variables, methods, and namespaces should use camel case with first letter lowercase (e.g., localFooBar).
Non-magic-number global variables that are non-static and not defined in anonymous namespace should use camel case prefixed by 'g' (e.g., gDontUseGlobalFoos).
Non-magic-number global variables that are static or defined in an anonymous namespace should use camel case prefixed by 's' (e.g., sMutableStaticGlobal).
Locally visible static variables should use camel case with lowercase prefix 's' as the first letter (e.g., static std::once_flag sFlag;).
Class member variables should use camel case prefixed with 'm' (e.g., mNbFooValues). Public member variables do not require the 'm' prefix but it is encouraged for clarity.
Enumerations, global constants, static constants at class-scope, and function-scope magic-number/literal constants should be uppercase snake case with prefix...
Files:
cpp/include/tensorrt_llm/common/stringUtils.hcpp/tensorrt_llm/common/stringUtils.cpp
**/*.{h,hpp}
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
Use a preprocessor guard in header files. The guard name must have prefix TRTLLM_ followed by the filename, all in caps, and no trailing underscore.
Files:
cpp/include/tensorrt_llm/common/stringUtils.h
**/*.{cpp,h,hpp,cc,cxx,cu,py}
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
All TensorRT-LLM Open Source Software code should contain an NVIDIA copyright header that includes the current year. This includes .cpp, .h, .cu, .py, and any other source files which are compiled or interpreted.
Files:
cpp/include/tensorrt_llm/common/stringUtils.hcpp/tensorrt_llm/common/stringUtils.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: fyf2016
PR: NVIDIA/TensorRT-LLM#6679
File: cpp/include/tensorrt_llm/common/logger.h:63-68
Timestamp: 2025-08-07T01:45:23.301Z
Learning: In C++ variadic template functions, the compiler implicitly inserts positional placeholders for template parameter packs during __attribute__((format(printf, ...))) processing, causing an offset in parameter numbering. The format string parameter index should account for this template expansion mechanism rather than the nominal parameter position.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit Check
🔇 Additional comments (5)
cpp/include/tensorrt_llm/common/stringUtils.h (2)
77-104: LGTM! Well-implemented RAII wrapper for va_list management.The
va_list_guardclass correctly manages the lifetime of ava_listusing RAII principles. The implementation properly:
- Takes a reference to avoid copying the va_list
- Calls
va_endin the destructor for automatic cleanup- Prevents copying and moving to maintain single ownership
- Deletes the rvalue
get()to prevent use-after-move scenarios
143-165: Excellent exception-safe implementation using RAII.The refactored
fmtstrfunction properly leverages theva_list_guardwrapper to ensure exception safety. The wrapper's destructor will automatically callva_endeven iffmtstr_throws an exception, addressing the reviewer's concern about exception safety.cpp/tensorrt_llm/common/stringUtils.cpp (3)
29-38: Well-implemented automatic cleanup for duplicated va_list.The function correctly:
- Creates a copy of the va_list using
va_copy- Wraps it in a
va_list_guardfor automatic cleanup- Uses the guard's
get()method to access the underlying va_listThis ensures that
va_endis called onargs0whenargs0_guardgoes out of scope, fixing the original resource leak issue.
53-54: Correct usage of the original va_list through the guard.The code properly uses
args_guard.get()to access the original va_list for the secondvsnprintfcall when the buffer needs to be reallocated. The RAII wrapper ensures proper cleanup regardless of the execution path.
76-76: Missing comment for namespace closing brace.According to the coding guidelines, closing braces of namespaces should have a comment indicating the namespace being closed.
-} // namespace tensorrt_llm::common +} // namespace tensorrt_llm::commonWait, the comment is already there. This is correct.
Signed-off-by: fanyunfan <[email protected]>
Signed-off-by: fanyunfan <[email protected]>
|
/bot run |
|
PR_Github #15005 [ run ] triggered by Bot |
|
PR_Github #15005 [ run ] completed with state |
Thank you for reviewing my code even though it's so late. I really appreciate it. |
|
/bot run |
|
PR_Github #15048 [ run ] triggered by Bot |
|
PR_Github #15048 [ run ] completed with state |
|
/bot run |
|
PR_Github #15209 [ run ] triggered by Bot |
|
/bot run |
|
PR_Github #15272 [ run ] triggered by Bot |
|
PR_Github #15272 [ run ] completed with state |
|
/bot run |
|
PR_Github #15377 [ run ] triggered by Bot |
|
PR_Github #15377 [ run ] completed with state |
…ease of va_list for potential resource leak (NVIDIA#6758) Signed-off-by: fanyunfan <[email protected]> Co-authored-by: fanyunfan <[email protected]> Co-authored-by: Yunfan Fan <[email protected]> Co-authored-by: Yuan Tong <[email protected]>
…ease of va_list for potential resource leak (NVIDIA#6758) Signed-off-by: fanyunfan <[email protected]> Co-authored-by: fanyunfan <[email protected]> Co-authored-by: Yunfan Fan <[email protected]> Co-authored-by: Yuan Tong <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…ease of va_list for potential resource leak (NVIDIA#6758) Signed-off-by: fanyunfan <[email protected]> Co-authored-by: fanyunfan <[email protected]> Co-authored-by: Yunfan Fan <[email protected]> Co-authored-by: Yuan Tong <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…ease of va_list for potential resource leak (NVIDIA#6758) Signed-off-by: fanyunfan <[email protected]> Co-authored-by: fanyunfan <[email protected]> Co-authored-by: Yunfan Fan <[email protected]> Co-authored-by: Yuan Tong <[email protected]>
…ease of va_list for potential resource leak (NVIDIA#6758) Signed-off-by: fanyunfan <[email protected]> Co-authored-by: fanyunfan <[email protected]> Co-authored-by: Yunfan Fan <[email protected]> Co-authored-by: Yuan Tong <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…ease of va_list for potential resource leak (NVIDIA#6758) Signed-off-by: fanyunfan <[email protected]> Co-authored-by: fanyunfan <[email protected]> Co-authored-by: Yunfan Fan <[email protected]> Co-authored-by: Yuan Tong <[email protected]>
…ease of va_list for potential resource leak (NVIDIA#6758) Signed-off-by: fanyunfan <[email protected]> Co-authored-by: fanyunfan <[email protected]> Co-authored-by: Yunfan Fan <[email protected]> Co-authored-by: Yuan Tong <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…ease of va_list for potential resource leak (NVIDIA#6758) Signed-off-by: fanyunfan <[email protected]> Co-authored-by: fanyunfan <[email protected]> Co-authored-by: Yunfan Fan <[email protected]> Co-authored-by: Yuan Tong <[email protected]>
…ease of va_list for potential resource leak (NVIDIA#6758) Signed-off-by: fanyunfan <[email protected]> Co-authored-by: fanyunfan <[email protected]> Co-authored-by: Yunfan Fan <[email protected]> Co-authored-by: Yuan Tong <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…ease of va_list for potential resource leak (NVIDIA#6758) Signed-off-by: fanyunfan <[email protected]> Co-authored-by: fanyunfan <[email protected]> Co-authored-by: Yunfan Fan <[email protected]> Co-authored-by: Yuan Tong <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…ease of va_list for potential resource leak (NVIDIA#6758) Signed-off-by: fanyunfan <[email protected]> Co-authored-by: fanyunfan <[email protected]> Co-authored-by: Yunfan Fan <[email protected]> Co-authored-by: Yuan Tong <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…ease of va_list for potential resource leak (NVIDIA#6758) Signed-off-by: fanyunfan <[email protected]> Co-authored-by: fanyunfan <[email protected]> Co-authored-by: Yunfan Fan <[email protected]> Co-authored-by: Yuan Tong <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…ease of va_list for potential resource leak (NVIDIA#6758) Signed-off-by: fanyunfan <[email protected]> Co-authored-by: fanyunfan <[email protected]> Co-authored-by: Yunfan Fan <[email protected]> Co-authored-by: Yuan Tong <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…ease of va_list for potential resource leak (NVIDIA#6758) Signed-off-by: fanyunfan <[email protected]> Co-authored-by: fanyunfan <[email protected]> Co-authored-by: Yunfan Fan <[email protected]> Co-authored-by: Yuan Tong <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
4 participants
Problem
The fmtstr_ function in the current implementation(
cpp/tensorrt_llm/common/stringUtils.cpp) uses va_copy(args0, args) to duplicate the va_list for safe usage in vsnprintf. However, it does not call va_end(args0) to clean up the copied va_list, which can lead to:Resource leaks on certain platforms where va_list requires explicit cleanup.
Undefined behavior (UB) per the C/C++ standard, since va_copy must always be paired with va_end.
Current Implementation
cpp/tensorrt_llm/common/stringUtils.cppSolution
Add va_end(args0) after vsnprintf finishes using args0. This ensures:
Compliance with the C/C++ standard (va_copy + va_end pairing).
No resource leaks on platforms where va_list manages allocated memory (e.g., some ABIs for variadic functions).
Summary by CodeRabbit
Bug Fixes
Refactor