Skip to content

check for valid size in cuOptGetErrorString #1414

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rapids-bot merged 1 commit into from
Jun 10, 2026
Merged

check for valid size in cuOptGetErrorString #1414

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cpp/src/pdlp/cuopt_c.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1174,6 +1174,7 @@ cuopt_int_t cuOptGetErrorString(cuOptSolution solution,
{
if (solution == nullptr) { return CUOPT_INVALID_ARGUMENT; }
if (error_string_ptr == nullptr) { return CUOPT_INVALID_ARGUMENT; }
if (error_string_size < 0) { return CUOPT_INVALID_ARGUMENT; }

@coderabbitai coderabbitai Bot Jun 10, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Inconsistency: validation should reject zero in addition to negative sizes.

The check < 0 allows error_string_size == 0, but this conflicts with:

  1. PR objectives: Issues #1311, #1313, and #1403 explicitly require rejecting "non-positive sizes (<= 0)".
  2. Existing pattern: cuOptGetParameter at line 882 rejects parameter_value_size <= 0, not just < 0.

A zero-size buffer is technically valid for std::snprintf but not useful—callers receive an unmodified buffer. For consistency with requirements and the existing validation pattern in this file, the check should be error_string_size <= 0.

🔧 Align validation with requirements and existing pattern 
- if (error_string_size < 0) { return CUOPT_INVALID_ARGUMENT; }
+ if (error_string_size <= 0) { return CUOPT_INVALID_ARGUMENT; }
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cpp/src/pdlp/cuopt_c.cpp` at line 1177, Update the validation for the error
string buffer size in cuopt_c.cpp so it rejects non-positive sizes: change the
check that currently tests error_string_size < 0 to test error_string_size <= 0;
this makes the behavior consistent with cuOptGetParameter (which uses
parameter_value_size <= 0) and the PR requirements to reject zero-sized buffers
when validating error_string_size in the relevant function.

@mlubin mlubin Jun 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a big deal. We just want to avoid the dangerous case where a negative size is passed to snprintf.

@coderabbitai coderabbitai Bot Jun 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mlubin, understood — the PR description already notes that zero is technically acceptable, and the fix targets only the unsafe negative case. That makes sense. Acknowledged!

✏️ Learnings added 
Learnt from: mlubin
Repo: NVIDIA/cuopt PR: 1414
File: cpp/src/pdlp/cuopt_c.cpp:1177-1177
Timestamp: 2026-06-10T01:28:36.327Z
Learning: In NVIDIA/cuopt `cpp/src/pdlp/cuopt_c.cpp`, `cuOptGetErrorString` intentionally uses `error_string_size < 0` (not `<= 0`) for buffer-size validation. A zero-size buffer is considered technically acceptable (std::snprintf handles it safely), so only negative values are rejected as `CUOPT_INVALID_ARGUMENT`. Do not flag this as inconsistent with `cuOptGetParameter`'s `<= 0` check.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used 
Learnt from: aliceb-nv
Repo: NVIDIA/cuopt PR: 986
File: cpp/src/branch_and_bound/branch_and_bound.cpp:8-8
Timestamp: 2026-03-23T11:33:23.998Z
Learning: In this repo (NVIDIA/cuopt), treat nvcc as the supported CUDA toolchain; clang-based compilation/support is not required and may fail/break. During code reviews, do NOT request code changes or add blocking comments for errors that appear only under clang (e.g., header-resolution failures such as 'utilities/determinism_log.hpp not found')—these can be toolchain-related rather than real source issues.

Learnt from: bdice
Repo: NVIDIA/cuopt PR: 1035
File: cpp/tests/utilities/base_fixture.hpp:29-47
Timestamp: 2026-04-19T16:49:17.616Z
Learning: In NVIDIA/cuopt (and the upstream rapidsai/rmm) after the RMM → CCCL migration, the rmm::mr resource adaptors (e.g., rmm::mr::pool_memory_resource and rmm::mr::binning_memory_resource) are now owning: they take/hold their upstream resources by value instead of non-owning references. Therefore, direct construction of adaptor chains from temporaries/local values (e.g., pool_memory_resource(make_async(), size)) is safe and should NOT be flagged as potential dangling/lifetime bugs. Also, rmm::mr::make_owning_wrapper is no longer needed/available for this owning design, so do not suggest it in this codepath.

solution_and_stream_view_t* solution_and_stream_view =
static_cast<solution_and_stream_view_t*>(solution);
std::string error_string = solution_and_stream_view->get_solution()->get_error_status().what();
Expand Down