Skip to content

Closing handles, freeing memory and preventing unneeded file opens #149

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
thomaslaurenson merged 1 commit into from
Mar 22, 2026
Merged

Closing handles, freeing memory and preventing unneeded file opens #149

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 5 additions & 8 deletions src/mpq.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ bool FileExistsInArchiveForLocale(const HANDLE hArchive, const std::string& file
if (fileLocale == locale) {
fileExists = true;
}
SFileCloseFile(hFile);
}
SFileCloseFile(hFile);
return fileExists;
}

Expand Down Expand Up @@ -79,6 +79,7 @@ int ExtractFiles(HANDLE hArchive, const std::string& output, const std::string&
findHandle,
&findData));

SFileFindClose(findHandle);
return result;
}

Expand All @@ -95,12 +96,6 @@ int ExtractFile(HANDLE hArchive, const std::string& output, const std::string& f
return 1;
}

HANDLE hFile;
if (!SFileOpenFileEx(hArchive, szFileName, SFILE_OPEN_FROM_MPQ, &hFile)) {
std::cerr << "[!] Failed: File cannot be opened: " << szFileName << std::endl;
return 1;
}

Comment on lines -98 to -103
Copy link
Copy Markdown
Contributor Author

@sjoblomj sjoblomj Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The call to SFileExtractFile below already does this under the hood. These lines don't add anything I believe.

Copy link
Copy Markdown
Collaborator

@thomaslaurenson thomaslaurenson Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good change - and this also fixes when successfully extracting a file, and the original hFile was never closed. I only noticed this when playing with Valgrind and doing more analysis. So a great fix!

// Change forward slashes on non-Windows systems
fs::path fileNamePath(fileName);
std::string fileNameString = NormalizeFilePath(fileNamePath);
Expand Down Expand Up @@ -352,7 +347,7 @@ int ListFiles(HANDLE hArchive, const std::string& listfileName, bool listAll, bo
// Multiple files can be stored with identical filenames under different locales.
// Loop over all locales and print the file details for each locale.
DWORD maxLocales = 32; // This will be updated in the call to SFileEnumLocales
LCID * fileLocales = (LCID *)malloc(maxLocales * sizeof(LCID));
const auto fileLocales = static_cast<LCID *>(malloc(maxLocales * sizeof(LCID)));
Comment on lines -355 to +350
Copy link
Copy Markdown
Contributor Author

@sjoblomj sjoblomj Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a cleaner way to allocate memory.

Copy link
Copy Markdown
Collaborator

@thomaslaurenson thomaslaurenson Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed - this is a good fix


if (fileLocales == NULL) {
std::cerr << "[!] Unable to allocate memory for locales for file: " << findData.cFileName << std::endl;
Expand All @@ -370,6 +365,7 @@ int ListFiles(HANDLE hArchive, const std::string& listfileName, bool listAll, bo

} else if (result == ERROR_INVALID_HANDLE || result == ERROR_NOT_SUPPORTED) {
std::cerr << "[!] Internal error for file: " << findData.cFileName << std::endl;
free(fileLocales);
continue;

} else if (result == ERROR_INSUFFICIENT_BUFFER) {
Expand Down Expand Up @@ -469,6 +465,7 @@ int ListFiles(HANDLE hArchive, const std::string& listfileName, bool listAll, bo

} while (SFileFindNextFile(findHandle, &findData));

SFileFindClose(findHandle);
return 0;
}

Expand Down