Skip to content

F-644: fix read of exactly MAX_LEN bytes treated as error#214

Open
miyazakh wants to merge 2 commits intowolfSSL:mainfrom
miyazakh:f-644_readofMAXLEN
Open

F-644: fix read of exactly MAX_LEN bytes treated as error#214
miyazakh wants to merge 2 commits intowolfSSL:mainfrom
miyazakh:f-644_readofMAXLEN

Conversation

@miyazakh
Copy link
Copy Markdown
Contributor

@miyazakh miyazakh commented Mar 27, 2026

Fix "Successful read of exactly MAX_LEN bytes treated as error"
Add test coverage

Depend on : #211 (Fixed)
Depend on : #219

Copilot AI review requested due to automatic review settings March 27, 2026 05:22
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes a decryption regression in the non-EVP crypto path where a successful XFREAD() returning exactly MAX_LEN bytes was incorrectly treated as an error, and extends shell test coverage to catch the boundary condition.

Changes:

  • Adjust wolfCLU_decrypt() to treat any positive-length XFREAD() result (including exactly MAX_LEN) as success.
  • Add an encrypt/decrypt round-trip test (Camellia, non-EVP path) that exercises multi-chunk reads.
  • Broaden OCSP interop test log matching for “invalid file” error text across implementations.

Reviewed changes

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

File Description
tests/ocsp/ocsp-interop-test.sh Expands expected error-message pattern matching for invalid certificate file test.
tests/encrypt/enc-test.sh Adds Camellia round-trip test intended to exercise MAX_LEN read boundary behavior in non-EVP decrypt path.
src/x509/clu_x509_sign.c Updates preprocessor gating for additional hash enum cases; whitespace-only adjustments nearby.
src/crypto/clu_decrypt.c Fixes handling of XFREAD() returning exactly MAX_LEN bytes so it is not treated as failure.

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

Comment on lines +188 to +190
# camellia: decrypt file larger than MAX_LEN (non-EVP path)
if grep -q "HAVE_CAMELLIA" wolfssl/wolfssl/options.h 2>/dev/null; then
dd if=/dev/urandom bs=2048 count=1 of=test_maxlen_camellia.bin 2>/dev/null
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

This regression is about handling a successful read of exactly MAX_LEN bytes. The test currently generates a 2048-byte input, which will exercise the code, but it also makes the intent less direct and bakes in an arbitrary size. Consider generating an input of exactly 1024 bytes (the current MAX_LEN) or documenting why 2048 is required, so the test remains clearly tied to the bug being fixed.

Suggested change
# camellia: decrypt file larger than MAX_LEN (non-EVP path)
if grep -q "HAVE_CAMELLIA" wolfssl/wolfssl/options.h 2>/dev/null; then
dd if=/dev/urandom bs=2048 count=1 of=test_maxlen_camellia.bin 2>/dev/null
# camellia: decrypt file of exactly MAX_LEN bytes (non-EVP path)
if grep -q "HAVE_CAMELLIA" wolfssl/wolfssl/options.h 2>/dev/null; then
dd if=/dev/urandom bs=1024 count=1 of=test_maxlen_camellia.bin 2>/dev/null

Copilot uses AI. Check for mistakes.
Comment on lines 158 to 162
ret = (int)XFREAD(input, 1, MAX_LEN, inFile);
if ((ret > 0 && ret != MAX_LEN) || feof(inFile)) {
if (ret > 0) {
tempMax = ret;
ret = 0; /* success */
}
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

Now that reads of exactly MAX_LEN are treated as success, the remaining failure case here is XFREAD() returning 0. Since that can mean either EOF or an underlying I/O error, it would be clearer to explicitly handle ret == 0 by checking feof()/ferror() after the read and logging an accurate message (instead of treating all 0-byte reads as the same error).

Copilot uses AI. Check for mistakes.
@miyazakh miyazakh self-assigned this Mar 27, 2026
@miyazakh miyazakh force-pushed the f-644_readofMAXLEN branch from 7e1ae69 to 764da36 Compare April 7, 2026 21:12
@miyazakh miyazakh marked this pull request as ready for review April 7, 2026 21:13
Copilot AI review requested due to automatic review settings April 7, 2026 21:13
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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


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

Comment on lines +159 to 171
if (ret > 0) {
tempMax = ret;
ret = 0; /* success */
}
else {
wolfCLU_LogError("Input file does not exist.");
if (feof(inFile)) {
wolfCLU_LogError("Unexpected end of file.");
}
else {
wolfCLU_LogError("File read error.");
}
ret = FREAD_ERROR;
}
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

This change fixes the ret == MAX_LEN case, but it also turns a clean EOF read (ret == 0 with feof(inFile) set) into an error. The previous logic treated feof(inFile) as a successful termination (with tempMax = 0). If this read is part of a loop that expects normal EOF, you likely need to preserve that behavior by treating ret == 0 && feof(inFile) as success/termination rather than FREAD_ERROR.

Copilot uses AI. Check for mistakes.

# camellia: decrypt file of exactly MAX_LEN bytes (non-EVP path)
if grep -q "HAVE_CAMELLIA" wolfssl/wolfssl/options.h 2>/dev/null; then
dd if=/dev/urandom bs=2048 count=1 of=test_maxlen_camellia.bin 2>/dev/null
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

The test claims to exercise an input of exactly MAX_LEN bytes, but it hard-codes 2048. If MAX_LEN changes, the test can silently stop covering the intended boundary. Consider deriving the length from the same source as MAX_LEN (e.g., extracting it from a header/config used by the build) or otherwise centralizing the value so the test remains aligned with the implementation.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants