Skip to content

HDDS-15480. Potential for NPE / infinite loop in StreamBlockReader#10431

Merged
smengcl merged 5 commits into
apache:masterfrom
sodonnel:HDDS-15480-npe
Jul 9, 2026
Merged

HDDS-15480. Potential for NPE / infinite loop in StreamBlockReader#10431
smengcl merged 5 commits into
apache:masterfrom
sodonnel:HDDS-15480-npe

Conversation

@sodonnel

@sodonnel sodonnel commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

There is potential for an NPE at stream shutdown in the StreamBlockReader:

poll() explicitly returns null when future.isDone() is true (stream completed). However readFromQueue() uses the return value without a null check:

  // StreamingReader.readFromQueue(), line 455-459
  ByteBuffer readFromQueue() throws IOException {
      final ReadBlockResponseProto readBlock = poll();   // can return null
      final ByteString data = readBlock.getData();       // NPE!

This fires in the normal shutdown path: after the server sends the last chunk and closes the stream, the next poll() call returns null and this line throws NullPointerException instead of letting the caller handle EOF gracefully.

Fixing this, can then cause an infinite loop in StreamingReader.read() (lines 446-451)

The inner read loop has no exit when readFromQueue() returns null or an empty buffer:

  while (true) {
      final ByteBuffer buf = readFromQueue();
      if (buf != null && buf.hasRemaining()) {
          return buf;     // only exit
      }
      // else: loop forever
  }

This could happen when:

  • Stream ends: once Bug 1 is fixed so readFromQueue() can return null, this loop spins forever on null.
  • Full offset skip: readFromQueue() adjusts the buffer position by pos - blockOffset. If the entire response arrived before the current position (e.g. after a seek, or when reading near the end of a small block), the adjusted buffer has 0 remaining bytes. The loop spins indefinitely without requesting new data, because readBlock() is only called once before the loop starts.

Note - this builds on #10430 and will need rebased after it goes in. done

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-15480

How was this patch tested?

Unit tests added to reproduce the problem and ensure the fix corrects them.

@ivandika3 ivandika3 requested review from Russole and szetszwo June 6, 2026 04:33
@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown

This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days.

@github-actions github-actions Bot added the stale label Jul 3, 2026
@adoroszlai adoroszlai requested a review from smengcl July 9, 2026 13:07
@smengcl smengcl marked this pull request as draft July 9, 2026 16:22
@smengcl smengcl added bug Something isn't working and removed stale labels Jul 9, 2026
Conflicts:
hadoop-hdds/client/src/test/java/org/apache/hadoop/hdds/scm/storage/TestStreamBlockInputStream.java

@smengcl smengcl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm. Thanks @sodonnel

@smengcl smengcl marked this pull request as ready for review July 9, 2026 21:24
Copilot AI review requested due to automatic review settings July 9, 2026 21:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes end-of-stream handling in StreamBlockInputStream’s streaming reader to prevent a NullPointerException on normal stream completion and to avoid an infinite loop when a response is fully skipped (eg after seek due to checksum-boundary alignment). It also adds unit tests that reproduce both the “fully-skipped first response” scenario and an EOF edge case where the stream completes before covering the seek position.

Changes:

  • Handle poll() / readFromQueue() returning null as EOF instead of dereferencing and throwing NPE.
  • Ensure the internal read() loop exits on stream end and continues fetching new responses when the current buffer is empty after skip.
  • Add unit tests covering checksum-aligned preamble skipping after seek and graceful EOF on premature stream completion.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/StreamBlockInputStream.java Prevents NPE on stream completion and avoids infinite looping when skipped responses yield empty buffers.
hadoop-hdds/client/src/test/java/org/apache/hadoop/hdds/scm/storage/TestStreamBlockInputStream.java Adds regression tests for checksum-alignment skip behavior and EOF handling when the stream completes early.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@smengcl smengcl merged commit a0f08cc into apache:master Jul 9, 2026
57 of 59 checks passed
@smengcl

smengcl commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Merged. Thanks @sodonnel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants