Bug #76670 GetLastError() should be called just after FindNextFile() at os0file.cc
Submitted: 13 Apr 2015 4:16 Modified: 14 Apr 2015 9:41
Reporter: Yasufumi Kinoshita Email Updates:
Status: Verified Impact on me:
None 
Category:MySQL Server: InnoDB storage engine Severity:S3 (Non-critical)
Version:5.6.23 OS:Microsoft Windows
Assigned to: CPU Architecture:Any

[13 Apr 2015 4:16] Yasufumi Kinoshita
Description:
At Windows code at storage/innobase/os/os0file.cc:os_file_readdir_next_file(),
GetLastError() is called after ut_free().

If the ut_free() clears the last error code, we meet the wrong error messages at .err file like...

2015-04-10 14:33:25 956 [ERROR] InnoDB: File (unknown): 'readdir_next_file' returned OS error 0.
2015-04-10 14:33:25 956 [ERROR] InnoDB: os_file_readdir_next_file() returned -1 in directory ., crash recovery may have failed for some .ibd files!
2015-04-10 14:33:25 956 [ERROR] InnoDB: File (unknown): 'readdir_next_file' returned OS error 0.
2015-04-10 14:33:25 956 [ERROR] InnoDB: os_file_readdir_next_file() returned -1 in directory ., crash recovery may have failed for some .ibd files!
.... (repeat 100 times for each database)

for recovery process of InnoDB.

How to repeat:
change to link the another memory allocator which clears the last error code at free().
(e.g. jemalloc for Windows. I tested mysql-5.6.23 + jemalloc-3.6.0)

And execute test which contains recovery process. (main.innodb_recovery_with_upper_case_names, innodb.blob_redo or innodb.innodb_bug59641)

Suggested fix:
store GetLastError() return value just after the FindNextFile() at os_file_readdir_next_file()
[14 Apr 2015 9:35] Mattias Jonsson
Looks like we could use the stack instead of a malloc to fix this issue:
diff --git a/storage/innobase/os/os0file.cc b/storage/innobase/os/os0file.cc
index fb7e8ca..ce2c103 100644
--- a/storage/innobase/os/os0file.cc
+++ b/storage/innobase/os/os0file.cc
@@ -892,31 +892,29 @@ os_file_readdir_next_file(
        os_file_stat_t* info)   /*!< in/out: buffer where the info is returned *
 {
 #ifdef __WIN__
-       LPWIN32_FIND_DATA       lpFindFileData;
+       WIN32_FIND_DATA         FindFileData;
        BOOL                    ret;

-       lpFindFileData = static_cast<LPWIN32_FIND_DATA>(
-               ut_malloc(sizeof(WIN32_FIND_DATA)));
 next_file:
-       ret = FindNextFile(dir, lpFindFileData);
+       ret = FindNextFile(dir, &FindFileData);

        if (ret) {
-               ut_a(strlen((char*) lpFindFileData->cFileName)
+               ut_a(strlen((char*) FindFileData.cFileName)
                     < OS_FILE_MAX_PATH);

-               if (strcmp((char*) lpFindFileData->cFileName, ".") == 0
-                   || strcmp((char*) lpFindFileData->cFileName, "..") == 0) {
+               if (strcmp((char*) FindFileData.cFileName, ".") == 0
+                   || strcmp((char*) FindFileData.cFileName, "..") == 0) {

                        goto next_file;
                }

-               strcpy(info->name, (char*) lpFindFileData->cFileName);
+               strcpy(info->name, (char*) FindFileData.cFileName);

-               info->size = (ib_int64_t)(lpFindFileData->nFileSizeLow)
-                       + (((ib_int64_t)(lpFindFileData->nFileSizeHigh))
+               info->size = (ib_int64_t)(FindFileData.nFileSizeLow)
+                       + (((ib_int64_t)(FindFileData.nFileSizeHigh))
                           << 32);

-               if (lpFindFileData->dwFileAttributes
+               if (FindFileData.dwFileAttributes
                    & FILE_ATTRIBUTE_REPARSE_POINT) {
                        /* TODO: test Windows symlinks */
                        /* TODO: MySQL has apparently its own symlink
@@ -924,7 +922,7 @@ next_file:
                        redirect a database directory:
                        REFMAN "windows-symbolic-links.html" */
                        info->type = OS_FILE_TYPE_LINK;
-               } else if (lpFindFileData->dwFileAttributes
+               } else if (FindFileData.dwFileAttributes
                           & FILE_ATTRIBUTE_DIRECTORY) {
                        info->type = OS_FILE_TYPE_DIR;
                } else {
@@ -936,8 +934,6 @@ next_file:
                }
        }

-       ut_free(lpFindFileData);
-
        if (ret) {
                return(0);
        } else if (GetLastError() == ERROR_NO_MORE_FILES) {
[14 Apr 2015 9:41] Umesh Shastry
Hello Yasufumi,

Thank you for the report, and Mattias for the quick patch.
Verifying based on internal discussion with Mattias, and code inspection.

Thanks,
Umesh
[14 Apr 2015 13:40] Mattias Jonsson
This is already fixed in 5.7, so it needs to be back ported to 5.6.