[None][fix] Fix: Add missing va_end(args0) in fmtstr_ to prevent potential resource leak #6758
Conversation
…rsion Signed-off-by: fanyunfan <[email protected]>
…ce leak Signed-off-by: fanyunfan <[email protected]>
📝 WalkthroughWalkthroughA call to Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ 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 comments)
Other keywords and placeholders
Documentation and Community
|
fyf2016
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. |
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