From 2276d94609d2acaf731551e5dfef1ad61cf13265 Mon Sep 17 00:00:00 2001 From: Yosuke Shimizu Date: Fri, 3 Apr 2026 08:38:15 +0900 Subject: [PATCH 1/2] Move out buffer allocation and Add cleanup phase for resource management --- src/wolfsftp.c | 135 +++++++++++++++++++++++++++++++++---------------- 1 file changed, 91 insertions(+), 44 deletions(-) diff --git a/src/wolfsftp.c b/src/wolfsftp.c index 01280df2e..75f70eccc 100644 --- a/src/wolfsftp.c +++ b/src/wolfsftp.c @@ -2026,6 +2026,8 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) word32 idx = 0; int m = 0; int ret = WS_SUCCESS; + int fdOpened = 0; + int outOwnedBySsh = 0; word32 outSz = sizeof(WFD) + UINT32_SZ + WOLFSSH_SFTP_HEADER; byte* out = NULL; @@ -2034,6 +2036,9 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) char ier[] = "Internal Failure"; char oer[] = "Open File Error"; char naf[] = "Not A File"; +#ifdef WOLFSSH_STOREHANDLE + int handleStored = 0; +#endif if (ssh == NULL) { return WS_BAD_ARGUMENT; @@ -2065,7 +2070,8 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) if (GetAndCleanPath(ssh->sftpDefaultPath, data + idx, sz, dir, sizeof(dir)) != WS_SUCCESS) { WLOG(WS_LOG_SFTP, "Creating path for file to open failed"); - return WS_FATAL_ERROR; + ret = WS_FATAL_ERROR; + goto cleanup; } idx += sz; @@ -2118,7 +2124,8 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) res = naf; if (wolfSSH_SFTP_CreateStatus(ssh, WOLFSSH_FTP_FAILURE, reqId, res, "English", NULL, &outSz) != WS_SIZE_ONLY) { - return WS_FATAL_ERROR; + ret = WS_FATAL_ERROR; + goto cleanup; } ret = WS_FATAL_ERROR; } @@ -2146,10 +2153,14 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) res = oer; if (wolfSSH_SFTP_CreateStatus(ssh, WOLFSSH_FTP_FAILURE, reqId, res, "English", NULL, &outSz) != WS_SIZE_ONLY) { - return WS_FATAL_ERROR; + ret = WS_FATAL_ERROR; + goto cleanup; } ret = WS_BAD_FILE_E; } + else { + fdOpened = 1; + } } #ifdef WOLFSSH_STOREHANDLE @@ -2160,58 +2171,59 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) res = ier; if (wolfSSH_SFTP_CreateStatus(ssh, WOLFSSH_FTP_FAILURE, reqId, res, "English", NULL, &outSz) != WS_SIZE_ONLY) { - #ifdef MICROCHIP_MPLAB_HARMONY - WFCLOSE(ssh->fs, &fd); - #else - WCLOSE(ssh->fs, fd); - #endif - return WS_FATAL_ERROR; + ret = WS_FATAL_ERROR; + goto cleanup; } ret = WS_FATAL_ERROR; } + else { + handleStored = 1; + } } #endif - if (ret == WS_SUCCESS) { - /* create packet */ - out = (byte*)WMALLOC(outSz, ssh->ctx->heap, DYNTYPE_BUFFER); - if (out == NULL) { - #ifdef MICROCHIP_MPLAB_HARMONY - WFCLOSE(ssh->fs, &fd); - #else - WCLOSE(ssh->fs, fd); - #endif - return WS_MEMORY_E; - } + /* create packet */ + out = (byte*)WMALLOC(outSz, ssh->ctx->heap, DYNTYPE_BUFFER); + if (out == NULL) { + ret = WS_MEMORY_E; + goto cleanup; } + if (ret == WS_SUCCESS) { if (SFTP_CreatePacket(ssh, WOLFSSH_FTP_HANDLE, out, outSz, (byte*)&fd, sizeof(WFD)) != WS_SUCCESS) { - #ifdef MICROCHIP_MPLAB_HARMONY - WFCLOSE(ssh->fs, &fd); - #else - WCLOSE(ssh->fs, fd); - #endif - return WS_FATAL_ERROR; + ret = WS_FATAL_ERROR; + goto cleanup; } } else { if (wolfSSH_SFTP_CreateStatus(ssh, WOLFSSH_FTP_FAILURE, reqId, res, "English", out, &outSz) != WS_SUCCESS) { - WFREE(out, ssh->ctx->heap, DYNTYPE_BUFFER); - if (fd >= 0) { - #ifdef MICROCHIP_MPLAB_HARMONY - WFCLOSE(ssh->fs, &fd); - #else - WCLOSE(ssh->fs, fd); - #endif - } - return WS_FATAL_ERROR; + ret = WS_FATAL_ERROR; + goto cleanup; } } /* set send out buffer, "out" is taken by ssh */ wolfSSH_SFTP_RecvSetSend(ssh, out, outSz); + outOwnedBySsh = 1; + +cleanup: +#ifdef WOLFSSH_STOREHANDLE + if (ret != WS_SUCCESS && handleStored) { + (void)SFTP_RemoveHandleNode(ssh, (byte*)&fd, sizeof(WFD)); + } +#endif + if (!outOwnedBySsh && out != NULL) { + WFREE(out, ssh->ctx->heap, DYNTYPE_BUFFER); + } + if (ret != WS_SUCCESS && fdOpened) { + #ifdef MICROCHIP_MPLAB_HARMONY + WFCLOSE(ssh->fs, &fd); + #else + WCLOSE(ssh->fs, fd); + #endif + } WOLFSSH_UNUSED(ier); return ret; @@ -2228,6 +2240,8 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) DWORD creationDisp = 0; DWORD flagsAndAttrs = 0; int ret = WS_SUCCESS; + int fileHandleOpened = 0; + int outOwnedBySsh = 0; word32 outSz = sizeof(HANDLE) + UINT32_SZ + WOLFSSH_SFTP_HEADER; byte* out = NULL; @@ -2235,6 +2249,11 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) char* res = NULL; char ier[] = "Internal Failure"; char oer[] = "Open File Error"; +#ifdef WOLFSSH_STOREHANDLE + int handleStored = 0; +#endif + + fileHandle = INVALID_HANDLE_VALUE; if (ssh == NULL) { return WS_BAD_ARGUMENT; @@ -2260,7 +2279,8 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) if (GetAndCleanPath(ssh->sftpDefaultPath, data + idx, sz, dir, sizeof(dir)) != WS_SUCCESS) { WLOG(WS_LOG_SFTP, "Creating path for file to open failed"); - return WS_FATAL_ERROR; + ret = WS_FATAL_ERROR; + goto cleanup; } idx += sz; @@ -2305,45 +2325,72 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) res = oer; if (wolfSSH_SFTP_CreateStatus(ssh, WOLFSSH_FTP_FAILURE, reqId, res, "English", NULL, &outSz) != WS_SIZE_ONLY) { - return WS_FATAL_ERROR; + ret = WS_FATAL_ERROR; + goto cleanup; } ret = WS_BAD_FILE_E; } + else { + fileHandleOpened = 1; + } #ifdef WOLFSSH_STOREHANDLE - if (SFTP_AddHandleNode(ssh, - (byte*)&fileHandle, sizeof(HANDLE), dir) != WS_SUCCESS) { + if (ret == WS_SUCCESS) { + if (SFTP_AddHandleNode(ssh, + (byte*)&fileHandle, sizeof(HANDLE), dir) != 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; + goto cleanup; } ret = WS_FATAL_ERROR; + } + else { + handleStored = 1; + } } #endif /* create packet */ out = (byte*)WMALLOC(outSz, ssh->ctx->heap, DYNTYPE_BUFFER); if (out == NULL) { - return WS_MEMORY_E; + ret = WS_MEMORY_E; + goto cleanup; } + if (ret == WS_SUCCESS) { if (SFTP_CreatePacket(ssh, WOLFSSH_FTP_HANDLE, out, outSz, (byte*)&fileHandle, sizeof(HANDLE)) != WS_SUCCESS) { - return WS_FATAL_ERROR; + ret = WS_FATAL_ERROR; + goto cleanup; } } else { if (wolfSSH_SFTP_CreateStatus(ssh, WOLFSSH_FTP_FAILURE, reqId, res, "English", out, &outSz) != WS_SUCCESS) { - WFREE(out, ssh->ctx->heap, DYNTYPE_BUFFER); - return WS_FATAL_ERROR; + ret = WS_FATAL_ERROR; + goto cleanup; } } /* set send out buffer, "out" is taken by ssh */ wolfSSH_SFTP_RecvSetSend(ssh, out, outSz); + outOwnedBySsh = 1; + +cleanup: +#ifdef WOLFSSH_STOREHANDLE + if (ret != WS_SUCCESS && handleStored) { + (void)SFTP_RemoveHandleNode(ssh, (byte*)&fileHandle, sizeof(HANDLE)); + } +#endif + if (!outOwnedBySsh && out != NULL) { + WFREE(out, ssh->ctx->heap, DYNTYPE_BUFFER); + } + if (ret != WS_SUCCESS && fileHandleOpened) { + CloseHandle(fileHandle); + } WOLFSSH_UNUSED(ier); return ret; From 494b1da1eba3893f1bbc6bb02cc6b110ef83a146 Mon Sep 17 00:00:00 2001 From: Yosuke Shimizu Date: Fri, 3 Apr 2026 10:48:59 +0900 Subject: [PATCH 2/2] Add test for non-existing file --- scripts/sftp.test | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/scripts/sftp.test b/scripts/sftp.test index 414bb54ae..261810ac8 100755 --- a/scripts/sftp.test +++ b/scripts/sftp.test @@ -139,6 +139,32 @@ if [ $nonblockingOnly = 0 ]; then fi fi +# Test SFTP status failure for non-existing file +if [ $nonblockingOnly = 0 ]; then + echo "Test SFTP status failure for non-existing file" + ./examples/echoserver/echoserver -N -1 -R $ready_file & + server_pid=$! + create_port + ./examples/sftpclient/wolfsftp -N -u jill -P upthehill -p $port -G -r $PWD/this_file_does_not_exist_12345.txt -l $PWD/test_output.txt > sftp_status_failure.log 2>&1 + RESULT=$? + grep "Unable to copy remote file" sftp_status_failure.log > /dev/null + RESULT_MSG=$? + remove_ready_file + rm -f sftp_status_failure.log + rm -f $PWD/test_output.txt + if [ $RESULT -eq 0 ]; then + echo -e "\n\nERROR: Should have failed for non-existing file" + do_cleanup + exit 1 + fi + if [ $RESULT_MSG -ne 0 ]; then + echo -e "\n\nERROR: Unexpected failure path while testing missing remote file" + do_cleanup + exit 1 + fi + echo "Successfully received failure status packet" +fi + echo -e "\nALL Tests Passed" exit 0