Skip to content
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.

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

[CABMAN] Fix a problem with creating temp files on some Windows system #514

Merged
merged 1 commit into from Dec 29, 2018

Conversation

tkreuzer
Copy link
Contributor

@tkreuzer tkreuzer self-assigned this Apr 22, 2018
FileHandle = fopen(tmpnam(NULL) + 1, "wb");
if (FileHandle == NULL)
return CAB_STATUS_CANNOT_CREATE;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really know what causes this breakage?

Copy link
Member

Choose a reason for hiding this comment

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

first tmpfile() fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason tmpfile() fails. Only happens with GCC builds, only on some Windows 10 systems. This fixes it for me. Everything else is left as an exersize to the reader ;-)

@sanchaez sanchaez added the bugfix For bugfix PRs. label Apr 23, 2018
@ColinFinck
Copy link
Member

I think merging this PR is still blocked by people not being able to reproduce the problem and not understanding the fix. And frankly spoken, it is hard to believe that tmpnam() always succeeds when tmpfile() fails.

For the record, MinGW builds of cabman.exe link against the OS msvcrt.dll to import tmpfile(). Visual Studio builds on the other hand statically link tmpfile() from their shipped CRT.
This can at least explain why only MinGW builds fail, and only on Windows 10, which may have introduced a bug in its tmpfile() implementation.

A minimal test to reproduce the tmpfile() problem may be the example code at https://msdn.microsoft.com/nl-nl/library/windows/desktop/x8x7sakw.aspx. @tkreuzer, does that trigger the problem for you as well?

Furthermore, the proposed fix does not handle a possible failure of tmpnam().
Additionally, it expects the returned filename to always be in the format "\tempfile123", indicating a temporary file in the current directory, and therefore skips the first slash. This does not hold when calling tmpnam() under Unix though (see http://en.cppreference.com/w/cpp/io/c/tmpnam).

We either need to understand the root cause and try to fix that. Or, if that is not possible or too boring, the code needs to be rewritten to not call tmp* CRT functions at all.
Either way, replacing one buggy tmp* function by another tmp* function is no option due to the aforementioned reasons.

Copy link
Member

@ColinFinck ColinFinck left a comment

Choose a reason for hiding this comment

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

Requesting changes (see my previous comment)

@tkreuzer
Copy link
Contributor Author

Hmm. Looks like this PR is only blocked by ... @ColinFinck :)
I could now argue that all I claimed is that this fixes build for me, and whoever can't reproduce it, might just not have put enough effort into it, and whoever finds things hard to believe is free to call me a liar, but luckily it seems like MS has fixed the issue with the latest VS update, so this makes this PR obsolete.

@tkreuzer tkreuzer closed this Jun 30, 2018
@HBelusca
Copy link
Contributor

Interesting! Diffing old VS crt vs new VS crt one could help knowing what happened there 😃

@tkreuzer
Copy link
Contributor Author

Reopening, since it is still not fixed for me, not sure how and why it succeeded in between.

@tkreuzer tkreuzer reopened this Dec 29, 2018
@tkreuzer tkreuzer merged commit bcb0d7c into reactos:master Dec 29, 2018
@tkreuzer tkreuzer deleted the CORE-14431_Fix_cabman branch August 22, 2019 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix For bugfix PRs.
Projects
None yet
6 participants