SFTP file handle abstraction #875

JacobBarthelmeh · 2026-02-03T23:07:19Z

ZD20562

Copilot

Pull request overview

This pull request introduces a new file handle abstraction for the SFTP subsystem, replacing the old conditional WOLFSSH_STOREHANDLE mechanism with an always-on tracking system. Instead of sending raw file descriptors or handles to SFTP clients, the implementation now generates unique 8-byte IDs that are tracked internally in a linked list (WS_FILE_LIST), improving security and portability.

Changes:

Removed old WOLFSSH_STOREHANDLE conditional compilation and associated functions (SFTP_AddHandleNode, SFTP_RemoveHandleNode, SFTP_GetHandleNode, SFTP_FreeHandles)
Added new file handle tracking system with WS_FILE_LIST structure and helper functions (SFTP_AddFileHandle, SFTP_FindFileHandle, SFTP_RemoveFileHandle, SFTP_FreeAllFileHandles)
Unified RecvClose implementation across platforms (previously had separate Unix and Windows versions)
Updated all file operations (Open, Read, Write, Close, FSTAT, FSetSTAT) to use 8-byte handle IDs instead of raw file descriptors

Reviewed changes

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

File	Description
wolfssh/wolfsftp.h	Removed declarations for old WOLFSSH_STOREHANDLE functions (SFTP_AddHandleNode, SFTP_RemoveHandleNode)
wolfssh/internal.h	Added WOLFSSH_HANDLE_ID_SZ constant, WS_FILE_LIST typedef, and fileIdCount/fileList members to WOLFSSH struct, removed conditional WOLFSSH_STOREHANDLE handleList member
src/wolfsftp.c	Implemented new file handle tracking system, updated all file operations to use 8-byte IDs, unified cross-platform RecvClose implementation, removed old WOLFSSH_STOREHANDLE code, fixed code style (else statement formatting)

Comments suppressed due to low confidence (1)

src/wolfsftp.c:2184

When SFTP_CreatePacket fails on line 2176, the file is closed (lines 2178-2182), but the file handle is not removed from the tracking list. This creates the same issue as with the malloc failure: the handle remains in ssh->fileList with an invalid file descriptor. SFTP_RemoveFileHandle should be called to clean up the tracking list before returning WS_FATAL_ERROR.

        if (SFTP_CreatePacket(ssh, WOLFSSH_FTP_HANDLE, out, outSz,
            idFlat, sizeof(idFlat)) != WS_SUCCESS) {
        #ifdef MICROCHIP_MPLAB_HARMONY
            WFCLOSE(ssh->fs, &fd);
        #else
            WCLOSE(ssh->fs, fd);
        #endif
            return WS_FATAL_ERROR;
        }

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

Copilot · 2026-02-03T23:13:14Z

src/wolfsftp.c

+        c32toa(id[1], idFlat + UINT32_SZ);
        if (SFTP_CreatePacket(ssh, WOLFSSH_FTP_HANDLE, out, outSz,
-            (byte*)&fileHandle, sizeof(HANDLE)) != WS_SUCCESS) {
+            idFlat, sizeof(idFlat)) != WS_SUCCESS) {


When SFTP_CreatePacket fails on line 2328, no cleanup is performed. The file handle remains in the tracking list and the file is not closed. This is similar to the Unix version's issue but lacks even the file close. CloseHandle should be called and SFTP_RemoveFileHandle should be invoked to clean up the tracking list before returning WS_FATAL_ERROR.

Suggested change

idFlat, sizeof(idFlat)) != WS_SUCCESS) {

idFlat, sizeof(idFlat)) != WS_SUCCESS) {

/* Cleanup on packet creation failure: close and remove handle,

and free buffer before returning a fatal error. */

CloseHandle(fileHandle);

(void)SFTP_RemoveFileHandle(ssh, id);

WFREE(out, ssh->ctx->heap, DYNTYPE_BUFFER);

Copilot · 2026-02-03T23:13:15Z

src/wolfsftp.c

+        /* close the file */
+        WCLOSE(ssh->fs, toFree->fd);


The SFTP_FreeAllFileHandles function uses WCLOSE unconditionally, but it should handle different platforms appropriately. For Windows (USE_WINDOWS_API), it should use CloseHandle instead of WCLOSE. For MICROCHIP_MPLAB_HARMONY, it should use WFCLOSE. This inconsistency with the platform-specific close operations used elsewhere (e.g., lines 4307-4314) will cause compilation or runtime errors on Windows and potentially on MICROCHIP_MPLAB_HARMONY platforms.

Suggested change

/* close the file */

WCLOSE(ssh->fs, toFree->fd);

/* close the file */

#ifdef USE_WINDOWS_API

CloseHandle(toFree->fd);

#elif defined(MICROCHIP_MPLAB_HARMONY)

WFCLOSE(ssh->fs, toFree->fd);

#else

WCLOSE(ssh->fs, toFree->fd);

#endif

Copilot · 2026-02-03T23:13:15Z

src/wolfsftp.c

        /* get offset into file */
        ato32(data + idx, &ofst[1]); idx += UINT32_SZ;


The offset parsing on lines 3789-3791 happens even when ret != WS_SUCCESS (i.e., when the file handle lookup failed). This means idx is incremented even when the file handle is invalid. While this doesn't cause immediate harm since the code checks ret before using fd, it's inconsistent with the error handling pattern and could lead to confusing behavior. The offset and length parsing should be inside a ret == WS_SUCCESS check or moved after the handle validation succeeds.

Copilot · 2026-02-03T23:13:15Z

src/wolfsftp.c

+        }



Line 3870 still contains a check for sz != sizeof(HANDLE) which is from the old implementation. This is redundant with the proper check on line 3879 and should be removed. Additionally, like the Unix version, the offset parsing on lines 3905-3912 happens even when ret != WS_SUCCESS (when file handle validation fails), which is inefficient and inconsistent.

Suggested change

}

}

}

if (ret == WS_SUCCESS) {

Copilot · 2026-02-03T23:13:16Z

src/wolfsftp.c

+/* hold pointers to file handles */
+struct WS_FILE_LIST {
+    WFD fd;        /* file descriptor */
+    char* fileName; /* base name of file */
+    word32 id[2];  /* handle ID */
+    struct WS_FILE_LIST* next;
+};


On Windows, SFTP_AddFileHandle is being passed a HANDLE type (fileHandle on line 2309), but the WS_FILE_LIST structure stores this in a WFD field (line 2363 in diff), which is defined as int on Windows (wolfssh/port.h line 1318). HANDLE is a pointer type (void*) on Windows which is 64-bit on x64 systems, while int is 32-bit. This will cause truncation and data loss on 64-bit Windows systems, leading to invalid file handles being stored and subsequently used. The WS_FILE_LIST structure should use HANDLE on Windows platforms, or a union type that can accommodate both int file descriptors and HANDLE pointers.

Copilot · 2026-02-03T23:13:17Z

src/wolfsftp.c

+                /* Close the file and remove from tracking list */
+                CloseHandle(fileNode->fd);
+#else
+                ret = WCLOSE(ssh->fs, fileNode->fd);
+#endif
+                if (ret >= 0) {


On Windows (line 4311), CloseHandle is called but the return value is not checked. Unlike the Unix/MICROCHIP paths (lines 4308, 4313), this doesn't set ret which means the check on line 4315 will use an undefined/stale value of ret. The Windows path should set ret = WS_SUCCESS (or check CloseHandle's return value properly) before the check on line 4315.

Copilot · 2026-02-03T23:13:17Z

src/wolfsftp.c

@@ -3903,8 +4125,43 @@ int wolfSSH_SFTP_RecvRead(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz)
    if (sz > maxSz - idx || sz > WOLFSSH_MAX_HANDLE || sz != sizeof(HANDLE)) {


Line 4125 still contains a check for sz != sizeof(HANDLE) which is from the old implementation where raw file handles were sent. With the new abstraction using 8-byte IDs, this check will fail because sizeof(HANDLE) is typically 4 or 8 bytes depending on platform but not necessarily equal to WOLFSSH_HANDLE_ID_SZ. This redundant check conflicts with the proper check on line 4130 and should be removed to avoid confusion and potential bugs.

Suggested change

if (sz > maxSz - idx || sz > WOLFSSH_MAX_HANDLE || sz != sizeof(HANDLE)) {

if (sz > maxSz - idx || sz > WOLFSSH_MAX_HANDLE) {

Copilot · 2026-02-03T23:13:17Z

src/wolfsftp.c

+    if (handleSz != (2 * sizeof(word32))) {
        WLOG(WS_LOG_SFTP, "Unexpected handle size for stored handles");
    }
    else {
-        WS_HANDLE_LIST* cur;
+        WS_FILE_LIST* cur;
+        word32 handleId[2] = {0, 0};

-        cur = SFTP_GetHandleNode(ssh, handle, handleSz);
+        ato32(handle, &handleId[0]);
+        ato32(handle + UINT32_SZ, &handleId[1]);

+        cur = SFTP_FindFileHandle(ssh, handleId);
        if (cur == NULL) {
            WLOG(WS_LOG_SFTP, "Unknown handle");
            return WS_BAD_FILE_E;
        }
-        name = cur->name;
+        name = cur->fileName;
    }


When handleSz != (2 * sizeof(word32)) on line 5270, the code logs a warning but continues execution without setting 'name'. This means the subsequent call to SFTP_GetAttributes_Handle on line 5290 will receive name=NULL (from its initialization on line 5248). This could lead to undefined behavior or incorrect results depending on how SFTP_GetAttributes_Handle handles NULL names. The code should either return an error when the handle size is wrong, or ensure that name is properly set through an alternative path.

Copilot · 2026-02-03T23:13:17Z

src/wolfsftp.c

    if (ret == WS_SUCCESS) {
-        WMEMSET((byte*)&fd, 0, sizeof(HANDLE));
-        WMEMCPY((byte*)&fd, data + idx, sz);
-        idx += sz;
+        /* Validate file handle size - must be 8 bytes for tracked handles */
+        if (sz != WOLFSSH_HANDLE_ID_SZ) {
+            WLOG(WS_LOG_SFTP, "Invalid file handle size - expected 8 bytes");
+            res = err;
+            type = WOLFSSH_FTP_FAILURE;
+            ret = WS_BAD_FILE_E;
+        }
+        else {
+            word32 handle[2] = {0, 0};
+            WS_FILE_LIST* fileEntry = NULL;
+
+            ato32(data + idx, &handle[0]);
+            ato32(data + idx + UINT32_SZ, &handle[1]);
+
+            fileEntry = SFTP_FindFileHandle(ssh, handle);
+            if (fileEntry == NULL) {
+                WLOG(WS_LOG_SFTP, "Invalid file handle - not found in session");
+                res  = err;
+                type = WOLFSSH_FTP_FAILURE;
+                ret  = WS_BAD_FILE_E;
+            }
+            else {
+                fd = (HANDLE)fileEntry->fd;
+            }
+            idx += sz;
+        }


The offset and length parsing (lines 3905-3919 outside changed region) happens unconditionally even when ret != WS_SUCCESS after the file handle validation fails. This is inefficient and could lead to using an uninitialized fd variable. The logic should guard operations with fd and parsing that depends on successful handle lookup with a ret == WS_SUCCESS check, similar to line 3799 in the Unix version.

Copilot · 2026-02-03T23:13:18Z

src/wolfsftp.c

+        if (SFTP_AddFileHandle(ssh, fileHandle, dir, id) != WS_SUCCESS) {
            WLOG(WS_LOG_SFTP, "Unable to store handle");
            res = ier;
            if (wolfSSH_SFTP_CreateStatus(ssh, WOLFSSH_FTP_FAILURE, reqId, res,
                "English", NULL, &outSz) != WS_SIZE_ONLY) {
                return WS_FATAL_ERROR;
            }
            ret = WS_FATAL_ERROR;
+        }
    }


When SFTP_AddFileHandle fails on line 2309, the file handle is not closed in the case where wolfSSH_SFTP_CreateStatus returns WS_SIZE_ONLY (line 2313). This creates a resource leak on Windows. CloseHandle should be called before setting ret to WS_FATAL_ERROR on line 2316.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

JacobBarthelmeh added 2 commits February 3, 2026 15:55

keep a list of file handles open per session

ad92290

node handle refactor

d192f8e

JacobBarthelmeh requested a review from Copilot February 3, 2026 23:07

JacobBarthelmeh self-assigned this Feb 3, 2026

Copilot started reviewing on behalf of JacobBarthelmeh February 3, 2026 23:07 View session

Copilot AI reviewed Feb 3, 2026

View reviewed changes

Copilot AI reviewed Feb 4, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SFTP file handle abstraction #875

SFTP file handle abstraction #875

JacobBarthelmeh commented Feb 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

Copilot AI Feb 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

-            idFlat, sizeof(idFlat)) != WS_SUCCESS) {
+            idFlat, sizeof(idFlat)) != WS_SUCCESS) {
+            /* Cleanup on packet creation failure: close and remove handle,
+               and free buffer before returning a fatal error. */
+            CloseHandle(fileHandle);
+            (void)SFTP_RemoveFileHandle(ssh, id);
+            WFREE(out, ssh->ctx->heap, DYNTYPE_BUFFER);

		/* get offset into file */
		ato32(data + idx, &ofst[1]); idx += UINT32_SZ;

		@@ -3903,8 +4125,43 @@ int wolfSSH_SFTP_RecvRead(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz)
		if (sz > maxSz - idx \|\| sz > WOLFSSH_MAX_HANDLE \|\| sz != sizeof(HANDLE)) {

SFTP file handle abstraction #875

Are you sure you want to change the base?

SFTP file handle abstraction #875

Conversation

JacobBarthelmeh commented Feb 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant