Skip to content

Validate cuOptGetErrorString buffer size - #1313

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

Validate cuOptGetErrorString buffer size#1313
fallintoplace wants to merge 1 commit into
NVIDIA:mainfrom
fallintoplace:fix/error-string-buffer-size

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

This tightens one C API edge case and adds a small regression test for it.

What changed:

  • reject non-positive buffer sizes in cuOptGetErrorString
  • add a focused C API test that checks 0 and -1 buffer sizes

Why:
cuOptGetParameter already rejects <= 0 output buffer sizes, but cuOptGetErrorString was still passing its signed size straight into snprintf. This just makes the behavior consistent and keeps the validation on the API boundary.

Testing:

  • git diff --check
  • not run here: this machine does not have nvcc or a configured cuOpt build environment, so I could not build libcuopt / run C_API_TEST locally

Closes #1311.

@copy-pr-bot

copy-pr-bot Bot commented May 27, 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.

@fallintoplace
fallintoplace marked this pull request as ready for review May 27, 2026 22:00
@fallintoplace
fallintoplace requested a review from a team as a code owner May 27, 2026 22:00
@fallintoplace
fallintoplace requested review from hlinsen and rg20 May 27, 2026 22:00
@coderabbitai

coderabbitai Bot commented May 27, 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: 340e3704-8491-4047-9983-b3fcd2e1f65d

📥 Commits

Reviewing files that changed from the base of the PR and between 86268f2 and f42ac18.

📒 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/src/pdlp/cuopt_c.cpp

📝 Walkthrough

Walkthrough

This PR adds input validation to the cuOptGetErrorString C API function to reject non-positive buffer sizes, updates the corresponding documentation, and introduces a test verifying the new validation behavior alongside successful error string retrieval with valid buffers.

Changes

Error String Buffer Size Validation

Layer / File(s) Summary
Implementation: buffer size validation in cuOptGetErrorString
cpp/src/pdlp/cuopt_c.cpp, cpp/include/cuopt/linear_programming/cuopt_c.h
Added an early-return guard in cuOptGetErrorString to return CUOPT_INVALID_ARGUMENT when error_string_size <= 0. Updated Doxygen documentation for the error_string_size parameter to state "Must be positive."
Test coverage for invalid buffer sizes
cpp/tests/linear_programming/c_api_tests/c_api_tests.cpp
Added test(c_api, error_string_invalid_buffer_size) that constructs a solution with a ValidationError, calls cuOptGetErrorString with invalid sizes (0, -1) expecting CUOPT_INVALID_ARGUMENT, and with a valid buffer expecting CUOPT_SUCCESS and the error message "validation failed". Includes cuopt/error.hpp to access error types.

🎯 1 (Trivial) | ⏱️ ~3 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 'Validate cuOptGetErrorString buffer size' accurately describes the main change in the pull request, which is adding validation for the buffer size parameter.
Description check ✅ Passed The description clearly explains what changed, why it was necessary, and references the related issue #1311. It is directly related to the changeset.
Linked Issues check ✅ Passed The pull request fully addresses all coding objectives from issue #1311: adds validation to reject error_string_size <= 0, prevents passing non-positive sizes to snprintf, and includes a regression test for both 0 and -1 buffer sizes.
Out of Scope Changes check ✅ Passed All changes are in scope and directly address the objectives from issue #1311. The modifications to cuopt_c.cpp, cuopt_c.h documentation, and the new test are all focused on the buffer size validation requirement.

✏️ 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:529:1-551: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.

@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown

🔔 Hi @anandhkb, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you!

If this is an "epic" issue, then please add the "epic" label to this issue.
If it is a PR and not ready for review, then please convert this to draft.
If you just want to switch off this notification, then use the "skip inactivity reminder" label.

@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

/ok to test bd69111

Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
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