Skip to content
This repository has been archived by the owner on Nov 9, 2017. It is now read-only.

Do not use _wfopen() for DOS device filenames #111

Merged
merged 1 commit into from
Jan 6, 2014
Merged

Conversation

dscho
Copy link
Member

@dscho dscho commented Jan 3, 2014

It appears that CONIN$ can be opened correctly by fopen(), but not
_wfopen() (with "CONIN$" passed in the correct format, of course).

Reported by Ray Kolbe.

It was also reported in msysgit/msysgit#153, for
some definition of "reported": it was not only reported to the wrong
repository but also so incomplete as to be contemptous of the time of the
people asked for help.

Signed-off-by: Johannes Schindelin johannes.schindelin@gmx.de

It appears that CONIN$ can be opened correctly by fopen(), but not
_wfopen() (with "CONIN$" passed in the correct format, of course).

Reported by Ray Kolbe.

It was also reported in msysgit/msysgit#153, for
some definition of "reported": it was not only reported to the wrong
repository but also so incomplete as to be contemptous of the time of the
people asked for help.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@buildhive
Copy link

MSysGit - the development behind Git for Windows » git #158 SUCCESS
This pull request looks good
(what's this?)

@dscho
Copy link
Member Author

dscho commented Jan 4, 2014

@sschuberth it does work with fopen, though. The problem with CreateFile (and you probably actually meant CreateFileW, didn't you?) is that it returns a HANDLE, not a FILE *, therefore we would have to override all of the FILE * functions in compat/mingw.c. That would be a huge undertaking and very fragile (because who knows what enterprisey upstream decides to use next in terms functions operating on FILE *?).

So even while that list is ugly (which is only the failure of the Win32 API to provide a function to query the list of reserved DOS device names), I am really hesitant to replace a functional, small work-around by an unwieldy, time-consuming and fragile bigger work-around.

dscho added a commit that referenced this pull request Jan 6, 2014
Do not use _wfopen() for DOS device filenames
@dscho dscho merged commit b08b38a into master Jan 6, 2014
@dscho
Copy link
Member Author

dscho commented Jan 6, 2014

I merged this with the intent of fixing the bug in the short run, and if it is too ugly (or too unmaintainable) dropping it in a future revision in favor of a cleaner solution.

@dscho dscho deleted the fix-https-prompt branch January 6, 2014 14:32
@kusma
Copy link
Member

kusma commented Jan 7, 2014

@dscho We wouldn't need to overload all functions operating on FILE-pointers, we only need to _open_osfhandle and _fdopen, no?

@dscho
Copy link
Member Author

dscho commented Jan 7, 2014

@kusma well, if we overload the fopen() function to use something internally that returns a HANDLE instead of a FILE *, we need to turn the former into the latter before we can return something valid... Therefore, _fdopen(_open_osfhandle(CreateFileW("CONIN$"))) should indeed do the job. I say "should" because I haven't tested it ;-)

@kblees
Copy link

kblees commented Jan 8, 2014

The culprit seems to be GetFullPathNameW converting CONIN$ to an absolute path (i.e. "C:\whatever\CONIN$"). I'd prefer to revert #86 instead of adding more workarounds.

@sschuberth
Copy link

Indeed, after reading thorough the issues again I agree with @kblees that we should revert both #86 and this one.

@dscho
Copy link
Member Author

dscho commented Jan 8, 2014

@sschuberth I agree. Maybe we give @vangdfang a chance to speak up before that?

@vangdfang
Copy link

I've reused the change here a bit in #113 to clean up the issue that #86 introduced. I agree, the string list isn't the nicest thing, but it's also perhaps the most accurate way to ensure we don't have a "special" DOS name.

dscho added a commit that referenced this pull request Jan 9, 2014
This reverts commit b08b38a, reversing
changes made to bac2422.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants