wininet: Always close the data connection before receiving a server response.

dlls/wininet/ftp.c

@@ -1365,15 +1365,16 @@ BOOL WINAPI FTP_FtpGetFileW(LPWININETFTPSESSIONW lpwfs, LPCWSTR lpszRemoteFile,
 
             /* Receive data */
             FTP_RetrieveFileData(lpwfs, nDataSocket, hFile);
+            closesocket(nDataSocket);
+
             nResCode = FTP_ReceiveResponse(lpwfs, dwContext);
             if (nResCode)
             {
                 if (nResCode == 226)
                     bSuccess = TRUE;
-		else
+                else
                     FTP_SetResponseError(nResCode);
             }
-	    closesocket(nDataSocket);
         }
     }
 
@@ -3008,7 +3009,7 @@ static void FTP_CloseFindNextHandle(LPWININETHANDLEHEADER hdr)
  *           FTP_CloseFileTransferHandle (internal)
  *
  * Closes the file transfer handle. This also 'cleans' the data queue of
- * the 'transfer conplete' message (this is a bit of a hack though :-/ )
+ * the 'transfer complete' message (this is a bit of a hack though :-/ )
  *
  */
 static void FTP_CloseFileTransferHandle(LPWININETHANDLEHEADER hdr)
@@ -3022,18 +3023,14 @@ static void FTP_CloseFileTransferHandle(LPWININETHANDLEHEADER hdr)
     WININET_Release(&lpwh->lpFtpSession->hdr);
 
     if (!lpwh->session_deleted)
-	lpwfs->download_in_progress = NULL;
-
-    /* This just serves to flush the control socket of any spurrious lines written
-       to it (like '226 Transfer complete.').
+        lpwfs->download_in_progress = NULL;
 
-       Wonder what to do if the server sends us an error code though...
-    */
-    nResCode = FTP_ReceiveResponse(lpwfs, lpwfs->hdr.dwContext);
-    
     if (lpwh->nDataSocket != -1)
         closesocket(lpwh->nDataSocket);
 
+    nResCode = FTP_ReceiveResponse(lpwfs, lpwfs->hdr.dwContext);
+    if (nResCode > 0 && nResCode != 226) WARN("server reports failed transfer\n");
+
     HeapFree(GetProcessHeap(), 0, lpwh);
 }
 

dlls/wininet/tests/ftp.c

@@ -301,16 +301,12 @@ static void test_getfile(void)
 
     /* Zero attributes */
     SetLastError(0xdeadbeef);
-    bRet = FtpGetFileA(hFtp, "welcome.msg", "should_be_non_existing_deadbeef", FALSE, 0, FTP_TRANSFER_TYPE_UNKNOWN, 0);
-    ok ( bRet == FALSE, "Expected FtpGetFileA to fail\n");
-    todo_wine
-    {
-    ok ( GetLastError() == ERROR_INTERNET_EXTENDED_ERROR,
-        "Expected ERROR_INTERNET_EXTENDED_ERROR, got %d\n", GetLastError());
-    ok (GetFileAttributesA("should_be_non_existing_deadbeef") == INVALID_FILE_ATTRIBUTES,
-        "Local file should not have been created\n");
-    }
-    DeleteFileA("should_be_non_existing_deadbeef");
+    bRet = FtpGetFileA(hFtp, "welcome.msg", "should_be_existing_non_deadbeef", FALSE, 0, FTP_TRANSFER_TYPE_UNKNOWN, 0);
+    ok ( bRet == TRUE, "Expected FtpGetFileA to succeed\n");
+    ok (GetLastError() == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %d\n", GetLastError());
+    ok (GetFileAttributesA("should_be_existing_non_deadbeef") != INVALID_FILE_ATTRIBUTES,
+        "Local file should have been created\n");
+    DeleteFileA("should_be_existing_non_deadbeef");
 
     /* Illegal condition flags */
     SetLastError(0xdeadbeef);
@@ -326,12 +322,14 @@ static void test_getfile(void)
     SetLastError(0xdeadbeef);
     bRet = FtpGetFileA(hFtp, "should_be_non_existing_deadbeef", "should_also_be_non_existing_deadbeef", FALSE, FILE_ATTRIBUTE_NORMAL, FTP_TRANSFER_TYPE_UNKNOWN, 0);
     ok ( bRet == FALSE, "Expected FtpGetFileA to fail\n");
+    todo_wine
+    {
     ok ( GetLastError() == ERROR_INTERNET_EXTENDED_ERROR,
         "Expected ERROR_INTERNET_EXTENDED_ERROR, got %d\n", GetLastError());
     /* Currently Wine always creates the local file (even on failure) which is not correct, hence the test */
-    todo_wine
     ok (GetFileAttributesA("should_also_be_non_existing_deadbeef") == INVALID_FILE_ATTRIBUTES,
         "Local file should not have been created\n");
+    }
     DeleteFileA("should_also_be_non_existing_deadbeef");
 
     /* Same call as the previous but now the local file does exists. Windows just removes the file if the call fails
@@ -346,32 +344,30 @@ static void test_getfile(void)
     SetLastError(0xdeadbeef);
     bRet = FtpGetFileA(hFtp, "should_be_non_existing_deadbeef", "should_also_be_non_existing_deadbeef", FALSE, FILE_ATTRIBUTE_NORMAL, FTP_TRANSFER_TYPE_UNKNOWN, 0);
     ok ( bRet == FALSE, "Expected FtpGetFileA to fail\n");
+    todo_wine
+    {
     ok ( GetLastError() == ERROR_INTERNET_EXTENDED_ERROR,
         "Expected ERROR_INTERNET_EXTENDED_ERROR, got %d\n", GetLastError());
     /* Currently Wine always creates the local file (even on failure) which is not correct, hence the test */
-    todo_wine
     ok (GetFileAttributesA("should_also_be_non_existing_deadbeef") == INVALID_FILE_ATTRIBUTES,
         "Local file should not have been created\n");
+    }
     DeleteFileA("should_also_be_non_existing_deadbeef");
 
-    /* This one should fail */
+    /* This one should succeed */
     SetLastError(0xdeadbeef);
-    bRet = FtpGetFileA(hFtp, "welcome.msg", "should_be_non_existing_deadbeef", FALSE, FILE_ATTRIBUTE_NORMAL, FTP_TRANSFER_TYPE_UNKNOWN, 0);
-    ok ( bRet == FALSE, "Expected FtpGetFileA to fail\n");
-    ok ( GetLastError() == ERROR_INTERNET_EXTENDED_ERROR,
-        "Expected ERROR_INTERNET_EXTENDED_ERROR, got %d\n", GetLastError());
+    bRet = FtpGetFileA(hFtp, "welcome.msg", "should_be_existing_non_deadbeef", FALSE, FILE_ATTRIBUTE_NORMAL, FTP_TRANSFER_TYPE_UNKNOWN, 0);
+    ok ( bRet == TRUE, "Expected FtpGetFileA to fail\n");
+    ok ( GetLastError() == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %d\n", GetLastError());
 
-    if (GetFileAttributesA("should_be_non_existing_deadbeef") != INVALID_FILE_ATTRIBUTES)
+    if (GetFileAttributesA("should_be_existing_non_deadbeef") != INVALID_FILE_ATTRIBUTES)
     {
         /* Should succeed as fFailIfExists is set to FALSE (meaning don't fail if local file exists) */
         SetLastError(0xdeadbeef);
         bRet = FtpGetFileA(hFtp, "welcome.msg", "should_be_non_existing_deadbeef", FALSE, FILE_ATTRIBUTE_NORMAL, FTP_TRANSFER_TYPE_UNKNOWN, 0);
-        todo_wine
-        {
         ok ( bRet == TRUE, "Expected FtpGetFileA to succeed\n");
         ok ( GetLastError() == ERROR_SUCCESS,
             "Expected ERROR_SUCCESS, got %d\n", GetLastError());
-        }
 
         /* Should fail as fFailIfExists is set to TRUE */
         SetLastError(0xdeadbeef);
@@ -387,7 +383,7 @@ static void test_getfile(void)
         ok ( GetLastError() == ERROR_FILE_EXISTS,
             "Expected ERROR_FILE_EXISTS, got %d\n", GetLastError());
 
-        DeleteFileA("should_be_non_existing_deadbeef");
+        DeleteFileA("should_be_existing_non_deadbeef");
     }
 
     InternetCloseHandle(hFtp);
@@ -478,13 +474,8 @@ static void test_openfile(void)
 
     SetLastError(0xdeadbeef);
     hOpenFile = FtpOpenFileA(hFtp, "welcome.msg", GENERIC_READ, FTP_TRANSFER_TYPE_ASCII, 0);
-    todo_wine
-    {
-    ok ( hOpenFile == NULL, "Expected FtpOpenFileA to fail\n");
-    /* For some strange/unknown reason, win98 returns ERROR_FILE_NOT_FOUND */
-    ok ( GetLastError() == ERROR_INTERNET_EXTENDED_ERROR || GetLastError() == ERROR_FILE_NOT_FOUND,
-        "Expected ERROR_INTERNET_EXTENDED_ERROR or ERROR_FILE_NOT_FOUND (win98), got %d\n", GetLastError());
-    }
+    ok ( hOpenFile != NULL, "Expected FtpOpenFileA to succeed\n");
+    ok ( GetLastError() == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %u\n", GetLastError());
 
     if (hOpenFile)
     {
@@ -840,14 +831,14 @@ static void test_multiple(void)
     }
 
     /* A correct call */
-    bRet = FtpGetFileA(hFtp, "welcome.msg", "should_be_non_existing_deadbeef", FALSE, FILE_ATTRIBUTE_NORMAL, FTP_TRANSFER_TYPE_UNKNOWN, 0);
-    DeleteFileA("should_be_non_existing_deadbeef");
+    bRet = FtpGetFileA(hFtp, "welcome.msg", "should_be_existing_non_deadbeef", FALSE, FILE_ATTRIBUTE_NORMAL, FTP_TRANSFER_TYPE_UNKNOWN, 0);
+    ok(bRet, "Expected FtpGetFileA to succeed\n");
+    DeleteFileA("should_be_existing_non_deadbeef");
 
     SetLastError(0xdeadbeef);
     hOpenFile = FtpOpenFileA(hFtp, "welcome.msg", GENERIC_READ, FTP_TRANSFER_TYPE_ASCII, 0);
-    ok ( hOpenFile == NULL, "Expected FtpOpenFileA to fail\n");
-    ok ( GetLastError() == ERROR_INTERNET_EXTENDED_ERROR || GetLastError() == ERROR_FILE_NOT_FOUND,
-        "Expected ERROR_INTERNET_EXTENDED_ERROR or ERROR_FILE_NOT_FOUND (win98), got %d\n", GetLastError());
+    ok(hOpenFile != NULL, "Expected FtpOpenFileA to succeed\n");
+    ok(GetLastError() == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %u\n", GetLastError());
 
     InternetCloseHandle(hOpenFile);
     InternetCloseHandle(hFtp);