Skip to content

Fix error string buffer size validation - #1403

Closed
Rohithmatham12 wants to merge 1 commit into
NVIDIA:mainfrom
Rohithmatham12:fix-error-string-buffer-size
Closed

Fix error string buffer size validation#1403
Rohithmatham12 wants to merge 1 commit into
NVIDIA:mainfrom
Rohithmatham12:fix-error-string-buffer-size

Conversation

@Rohithmatham12

Copy link
Copy Markdown

Summary

  • Reject non-positive error_string_size in cuOptGetErrorString before calling snprintf.
  • Add C API regression coverage for zero and negative buffer sizes using a valid CPU-backed solution handle.
  • Clarify that the error string buffer size must be positive.

Fixes #1311.

Testing

  • /opt/homebrew/opt/llvm/bin/clang-format -i cpp/src/pdlp/cuopt_c.cpp cpp/include/cuopt/linear_programming/cuopt_c.h cpp/tests/linear_programming/c_api_tests/c_api_tests.cpp
  • git diff --check

@Rohithmatham12
Rohithmatham12 requested a review from a team as a code owner June 8, 2026 16:09
@copy-pr-bot

copy-pr-bot Bot commented Jun 8, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 515cd824-04b6-47d8-ba22-d34b9bee8c64

📥 Commits

Reviewing files that changed from the base of the PR and between 4284d91 and 92cc477.

📒 Files selected for processing (3)
  • cpp/include/cuopt/linear_programming/cuopt_c.h
  • cpp/src/pdlp/cuopt_c.cpp
  • cpp/tests/linear_programming/c_api_tests/c_api_tests.cpp
✅ Files skipped from review due to trivial changes (1)
  • cpp/include/cuopt/linear_programming/cuopt_c.h
🚧 Files skipped from review as they are similar to previous changes (1)
  • cpp/tests/linear_programming/c_api_tests/c_api_tests.cpp

📝 Walkthrough

Walkthrough

The PR adds input validation to the C API function cuOptGetErrorString to reject non-positive error_string_size values. The implementation now returns CUOPT_INVALID_ARGUMENT when buffer size is zero or negative. Documentation and test coverage have been added to reflect this behavior.

Changes

Input Validation for cuOptGetErrorString

Layer / File(s) Summary
Input validation implementation and test coverage
cpp/include/cuopt/linear_programming/cuopt_c.h, cpp/src/pdlp/cuopt_c.cpp, cpp/tests/linear_programming/c_api_tests/c_api_tests.cpp
Doxygen documentation for cuOptGetErrorString is updated to explicitly document that error_string_size must be positive. Implementation adds an early guard to return CUOPT_INVALID_ARGUMENT when error_string_size <= 0. Test includes were added and a new gtest verifies rejection of zero and negative buffer sizes and success with a valid buffer size.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Fix error string buffer size validation' directly and concisely describes the main change: adding validation for the error_string_size parameter in cuOptGetErrorString before calling snprintf.
Description check ✅ Passed The description is clearly related to the changeset, explaining the summary of changes (validation addition, test coverage, documentation clarification) and referencing the linked issue #1311.
Linked Issues check ✅ Passed The PR fully addresses all coding requirements from issue #1311: validates error_string_size <= 0 in cuOptGetErrorString [#1311], adds C API regression tests for zero and negative buffer sizes [#1311], and clarifies buffer size must be positive in documentation [#1311].
Out of Scope Changes check ✅ Passed All changes are in-scope: the validation logic in cuopt_c.cpp, documentation update in cuopt_c.h, and test additions in c_api_tests.cpp directly address the requirements from issue #1311 without introducing unrelated modifications.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 Infer (1.2.0)
cpp/src/pdlp/cuopt_c.cpp

cpp/src/pdlp/cuopt_c.cpp:8:10: fatal error: 'cuopt/linear_programming/cuopt_c.h' file not found
8 | #include <cuopt/linear_programming/cuopt_c.h>
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
Aborting translation of method 'cuOptAddQuadraticConstraint' in file 'cpp/src/pdlp/cuopt_c.cpp': "Assert_failure src/clang/cAst_utils.ml:249:53"
Uncaught Internal Error: "Assert_failure src/clang/cAst_utils.ml:249:53"
Error backtrace:
Raised at ClangFrontend__CAst_utils.get_decl_from_typ_ptr in file "src/clang/cAst_utils.ml", line 249, characters 53-65
Called from ClangFrontend__CTrans.CTrans_funct.get_destructor_decl_ref in file "src/clang/cTrans.ml", line 658, characters 12-59
Called from ClangFrontend__CTrans.CTrans_funct.destructor_calls.(fun) in file "src/clang/cTrans.ml", line 2048, characters 12-69
Called from Base__List.rev_filter_map.loop in file "src/list.ml", line 944, characters 13-17
Called from Base__List.filter_map in file "src/list.ml" (inlined), line

... [truncated 2200 characters] ...

ClangFrontend__CTrans.CTrans_funct.instruction_insert_cxx_temporary_markers in file "src/clang/cTrans.ml", line 4866, characters 22-60
Called from ClangFrontend__CTrans.CTrans_funct.instruction_scope in file "src/clang/cTrans.ml", line 4850, characters 22-79
Called from ClangFrontend__CTrans.CTrans_funct.instruction_log.(fun) in file "src/clang/cTrans.ml", line 4782, characters 12-47
Re-raised at IStdlib__IExn.reraise_after in file "src/istd/IExn.ml" (inlined), line 13, characters 2-50
Called from ClangFrontend__CTrans.CTrans_funct.instruction_log.(fun) in file "src/clang/cTrans.ml", line 4784, characters 10-1023
Called from ClangFrontend__CTrans.CTrans_funct.instruction in file "src/clang/cTrans.ml" (inlined), line 4765, characters 38-71
Called from ClangFrontend__CTrans.CTrans_funct.exec

cpp/tests/linear_programming/c_api_tests/c_api_tests.cpp

In file included from cpp/tests/linear_programming/c_api_tests/c_api_tests.cpp:8:
cpp/tests/linear_programming/c_api_tests/c_api_tests.h:8:10: fatal error: 'cuopt/linear_programming/cuopt_c.h' file not found
8 | #include <cuopt/linear_programming/cuopt_c.h>
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
cpp/tests/linear_programming/c_api_tests/c_api_tests.cpp:526:1-548:1: ERROR translating statement 'CompoundStmt'
Aborting translation of method 'anonymous_namespace_cpp_tests_linear_programming_c_api_tests_c_api_tests.cpp::tcp_connect_check' in file 'cpp/tests/linear_programming/c_api_tests/c_api_tests.cpp': "Assert_failure src/clang/cAst_utils.ml:249:53"
Uncaught Internal Error: "Assert_failure src/clang/cAst_utils.ml:249:53"
Error backtrace:
Raised at ClangFrontend__CAst_utils.get_decl_from_typ_ptr in file "src/clang/cAst_utils.ml", line 249, characters 53-65
Called from ClangFrontend__CTrans.CTrans_funct.get_destructor_decl_ref in file "src/clang/cTrans.

... [truncated 2200 characters] ...

file "src/istd/IExn.ml" (inlined), line 18, characters 15-63
Called from ClangFrontend__CFrontend_errors.protect in file "src/clang/cFrontend_errors.ml", line 48, characters 6-141
Called from ClangFrontend__CFrontend_decl.CFrontend_decl_funct.add_method in file "src/clang/cFrontend_decl.ml" (inlined), line 54, characters 4-52
Called from ClangFrontend__CFrontend_decl.CFrontend_decl_funct.function_decl in file "src/clang/cFrontend_decl.ml", line 90, characters 12-151
Called from ClangFrontend__CFrontend_decl.CFrontend_decl_funct.translate_one_declaration in file "src/clang/cFrontend_decl.ml", line 453, characters 10-56
Called from ClangFrontend__CFrontend_decl.CFrontend_decl_funct.translate_one_declaration.translate in file "src/clang/cFrontend_decl.ml" (inlined), line 448, characters 24-9


Comment @coderabbitai help to get the list of available commands and usage tips.

@nguidotti nguidotti added bug Something isn't working non-breaking Introduces a non-breaking change mip labels Jun 9, 2026
@nguidotti nguidotti added this to the 26.08 milestone Jun 9, 2026
@nguidotti

Copy link
Copy Markdown
Contributor

There is a similar fix in #1313. Although the test case here seems better.

@nguidotti

Copy link
Copy Markdown
Contributor

/ok to test 4284d91

Signed-off-by: Rohithmatham12 <rohithmatham@gmail.com>
@Rohithmatham12
Rohithmatham12 force-pushed the fix-error-string-buffer-size branch from fb60cb2 to 92cc477 Compare June 9, 2026 23:26
@rapids-bot rapids-bot Bot closed this in #1414 Jun 10, 2026
pull Bot pushed a commit to Stars1233/cuopt that referenced this pull request Jun 10, 2026
snprintf takes a size_t, so we should be sure to not pass a negative size to it. A zero size is technically correct and not dangerous or invalid.

Fixes NVIDIA#1311
Closes NVIDIA#1313
Closes NVIDIA#1403

Authors:
  - Miles Lubin (https://github.com/mlubin)

Approvers:
  - Nicolas L. Guidotti (https://github.com/nguidotti)

URL: NVIDIA#1414
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working mip non-breaking Introduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] cuOptGetErrorString should reject non-positive buffer sizes

2 participants