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
• Status: Verified
• Category: MySQL Server: InnoDB storage engine
• Severity: S3 (Non-critical)
• Version: 5.6.23
• OS: Windows
• 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] MySQL Verification Team

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.