Skip to content

Add length check to cudf::strings::is_timestamp logic for literals - #23712

Open
davidwendt wants to merge 2 commits into
NVIDIA:mainfrom
davidwendt:fix-is-timestamp-invalid-read
Open

Add length check to cudf::strings::is_timestamp logic for literals#23712
davidwendt wants to merge 2 commits into
NVIDIA:mainfrom
davidwendt:fix-is-timestamp-invalid-read

Conversation

@davidwendt

Copy link
Copy Markdown
Contributor

Description

Adds a length check before comparing a literal within the given format against a string row value in the cudf::strings::is_timestamp() API. This prevents reading past the end of the string. Also added a test for this case as well. The other format specifiers (non-literal) already check the length correctly.

Closes #23660

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@davidwendt davidwendt self-assigned this Aug 18, 2026
@davidwendt
davidwendt requested a review from a team as a code owner August 18, 2026 21:02
@davidwendt davidwendt added the bug Something isn't working label Aug 18, 2026
@davidwendt davidwendt added 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change labels Aug 18, 2026
@coderabbitai

coderabbitai Bot commented Aug 18, 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: 066985fc-d63c-4fb9-93c8-5ff7127f774e

📥 Commits

Reviewing files that changed from the base of the PR and between 215888d and bfd09e8.

📒 Files selected for processing (2)
  • cpp/src/strings/convert/convert_datetime.cu
  • cpp/tests/strings/datetime_tests.cpp

Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review.


📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes
    • Improved timestamp validation for formats containing trailing literal characters.
    • Prevented invalid or incomplete timestamp inputs from being incorrectly accepted or causing unsafe reads.

Walkthrough

The timestamp parser now checks input length before matching literal characters. A regression test verifies that is_timestamp rejects input missing a trailing format literal.

Changes

Timestamp literal validation

Layer / File(s) Summary
Literal bounds check and regression test
cpp/src/strings/convert/convert_datetime.cu, cpp/tests/strings/datetime_tests.cpp
The literal matching path checks the remaining input length before dereferencing the input pointer. A test covers an input missing a trailing literal.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to bfd09

This localized fix adds a length check to prevent invalid reads and includes a regression test; no actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: pointkernel

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% 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
Linked Issues check ✅ Passed The code adds the required length guard and a regression test for issue #23660.
Out of Scope Changes check ✅ Passed The implementation and test are limited to the linked out-of-bounds read in is_timestamp literal handling.
Title check ✅ Passed The title clearly and concisely describes the added length check in cudf::strings::is_timestamp logic.
Description check ✅ Passed The description directly explains the length check, the out-of-bounds read prevention, and the added test.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@davidwendt davidwendt changed the title A length check to cudf::strings::is_timestamp logic for literals Add length check to cudf::strings::is_timestamp logic for literals Aug 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3 - Ready for Review Ready for review by team bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] is_timestamp reads out of bounds when a format literal follows the last field

2 participants