Skip to content

tests: fix api.test SFTP read target race - #1142

Open
ejohnstown wants to merge 3 commits into
wolfSSL:masterfrom
ejohnstown:api-test-fix
Open

tests: fix api.test SFTP read target race#1142
ejohnstown wants to merge 3 commits into
wolfSSL:masterfrom
ejohnstown:api-test-fix

Conversation

@ejohnstown

Copy link
Copy Markdown
Contributor
  • The test opened the listing's first regular file, which raced with tests/testsuite.test creating and removing files in the same directory under make -j check — the macOS -1062 flake. It now stages its own target over SFTP, selects it by name, and removes it afterward.
  • Staging retries on WS_WANT_READ/WS_WANT_WRITE/WS_REKEYING and clamps writes to land exactly on WOLFSSH_MAX_SFTP_RW for any value of that macro; the listing assert, read block, and cleanup all key on Open success, so reads never target a stale file.
  • Reads now verify the returned payload is the staged fill byte, not just a plausible length.

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 updates the SFTP read API test to eliminate a directory-entry selection race under parallel test execution by staging a dedicated read target file over SFTP and validating the read payload contents.

Changes:

  • Stage a deterministic SFTP read target file (write exactly WOLFSSH_MAX_SFTP_RW bytes) and select it by name from LS output.
  • Add a payload validator (SFTP_CHECK_READ_PAYLOAD) to ensure reads return the expected fill byte, not just a plausible length.
  • Clean up the staged read target after the test completes.
Suppressed comments (1)

tests/api.c:2833

  • The LS retry loop has the same issue as the create loop: if it hits the 1000-try limit due to repeated WS_WANT_READ/WS_WANT_WRITE/WS_REKEYING, the test proceeds with current==NULL (and may end up skipping the read path if rdCreated is also false). It’s better to fail on timeout so the test doesn’t silently pass when the SFTP session is stuck.

Suggestion: after the loop, if current is still NULL, assert the last error wasn’t WANT/REKEYING (i.e., ensure the loop didn’t time out on transient errors).

        current = NULL;
        for (rdTries = 0; rdTries < 1000; rdTries++) {
            current = wolfSSH_SFTP_LS(ssh, (char*)currentDir);
            if (current != NULL) {
                break;
            }
            rdErr = wolfSSH_get_error(ssh);
            if (rdErr != WS_WANT_READ && rdErr != WS_WANT_WRITE &&
                    rdErr != WS_REKEYING) {
                break;
            }
        }

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

Comment thread tests/api.c Outdated
Comment thread tests/api.c Outdated

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #1142

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (3)

tests/api.c:2932

  • Cleanup Remove is currently unchecked. If it returns WS_WANT_* / WS_REKEYING, the pending RM state can interfere with the following WOLFSSH_TEST_INTERNAL write-path coverage (and may leave the temp file behind). Consider retrying until the remove completes and asserting success.
        wolfSSH_SFTP_Remove(ssh, rdName);

tests/api.c:2783

  • wolfSSH_SFTP_Remove() can return WS_WANT_READ/WS_WANT_WRITE/WS_REKEYING and expects the caller to retry the same operation until it completes. Ignoring the return here can leave a pending RM state that may consume/expect packets while the following Open runs, reintroducing flakes.

This issue also appears on line 2932 of the same file.

        wolfSSH_SFTP_Remove(ssh, rdName); /* clear any stale file */

tests/api.c:2821

  • wolfSSH_SFTP_Close() is also a stateful SFTP operation and may return WS_WANT_READ/WS_WANT_WRITE/WS_REKEYING. Not checking/retrying it can leave a pending CLOSE response that interferes with the subsequent LS/read operations (same underlying stream).
        wolfSSH_SFTP_Close(ssh, rdHandle, rdHandleSz);

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #1142

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

- Opening the listing's first entry raced with tests/testsuite.test,
  which creates and removes files in the same directory under
  "make -j check"; the entry could be gone before the open.
- Stage the read target over SFTP, select it from the listing by name,
  and remove it afterward.
- Check the listing with AssertNotNull; a missing entry used to skip
  the whole test body.
- Clamp the staging write chunk to the bytes remaining and derive the
  try cap from the chunk count, so the loop lands exactly on
  WOLFSSH_MAX_SFTP_RW for any value of the macro.
- Retry the staging Open and the LS on WS_WANT_READ, WS_WANT_WRITE,
  and WS_REKEYING, matching the rekey tolerance used elsewhere in the
  function.
- Assert the staging Open, the LS, and the listing match rather than
  keying the read block on them, so neither a failed create nor a
  connection stuck in WANT/REKEYING until the try cap can drop the
  read coverage without failing the test.
- Remove any stale file before the staging Open, so the reads never
  target a file staged by a prior aborted run.
- Size the read buffer from WOLFSSH_MAX_SFTP_RW, the amount staged and
  the largest amount read, instead of the listed file size, and drop
  the listed-size guard on the 18 byte read.
- Guard the listing name compare against a NULL fName.
- Verify every read returns the staged fill byte, not just a valid
  length; the check is a macro so a failure reports the calling
  read's line.
- add sftp_retry_remove()/sftp_retry_close() so Close and Remove are
  driven to completion instead of abandoned mid-operation
- split the retry predicate: Open and SendWritePacket keep state only on
  WS_WANT_READ/WS_WANT_WRITE, LS adds WS_REKEYING, Close and Remove gate
  on NoticeError()
- assert the staging close; keep the post-read cleanup best effort, as
  the reads are skipped rather than completed on WS_REKEYING
- route the WOLFSSH_TEST_INTERNAL write cleanup through the helpers
- shrink the staging chunk to 512 bytes
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.

4 participants