Fix netrc parsing on Windows

[orgads]

Open it in text mode to avoid considering CR as part of the line

[dscho]

Excellent! (For context: I am the Git for Windows maintainer and would love to see this change integrated soon)

[jay]

Mode t isn't widely recognized. It's not necessary unless global _fmode was used to set the default mode to binary. Something has changed in the way you build curl that causes this problem, I think.

[orgads]

According to cygwin docs ("The default Cygwin behavior", section b), if the file path contains backslashes the default is binary. In netrc.c DIR_CHAR, which is a backslash on WIN32, is used as a separator, and curl_getenv might also return a path with backslashes.

[dscho]

Mode t isn't widely recognized.

That might be true (although all the Windows, Linux and MacOSX compilers I know -- GCC, clang, Visual C++, icc, Borland C -- support it, or at least handle it gracefully). However, it would benefit all Windows-based projects using cURL, as cURL would not have to be patched to support CR/LF line endings in $HOME/_netrc.

[bagder]

Unfortunately I believe "t" is not defined by POSIX: http://pubs.opengroup.org/onlinepubs/009695399/functions/fopen.html

... instead it says: "If the string is one of the following, the file shall be opened in the indicated mode. Otherwise, the behavior is undefined."

To play really safe, I would say that a little #ifdef dance would be appropriate there.

[jay]

Thanks, I was able to reproduce in cygwin. A modified version of your patch landed in 33058a1.

[jay]

@orgads Can you confirm this works for you? I read in the git-for-windows issue that someone else tested and it works for them but I would like to know about your specific case. It works for my Cygwin POSIX-only build but it looks like you're using a build that's a POSIX/WinAPI mix.

@bagder I'm thinking of making the same change for curlrc to handle the case where the build is Cygwin POSIX-only but the user's curlrc is CRLF, to ensure it's translated properly. Any objection?

[bagder]

@jay: sounds perfectly logical and fine to me

[orgads]

@jay: __CYGWIN__ doesn't seem to be defined in mingw. Change it to WIN32 maybe?

[jay]

Orgad I thought you were using Cygwin. How are you building? The only way I can reproduce is in Cygwin. The curl and libcurl packages available from the cygwin package manager are what I tested with. You seem to be using a build that's using both POSIX and the Windows API I think, because your netrc is "_netrc" and that underscore style is only for builds in Windows that aren't POSIX-only, which means libcurl uses the Windows API.

Using WIN32 by itself won't solve it since it isn't defined if POSIX-only; though I could use defined(__CYGWIN__) || defined(WIN32) which won't break anything. I'm still at a loss for what POSIX-only system is being used when WIN32 is defined. Can you tell me if the fix as it is now works for what you described in your original report, because if so that means __CYGWIN__ is defined when you build.

[orgads]

I'm building on msys2 environment. See PR there: https://github.com/Alexpux/MINGW-packages/pull/613

[jay]

I still am not able to reproduce this in msys2 with _netrc though I believe it's likely. So far:

I've installed msys2 using the 64-bit installer. There are 3 environments available in msys2: msys2 (64-bit), mingw32 (mingw-w64 32-bit) and mingw64 (mingw-w64 64-bit). mingw32 has /mingw32/bin first in the path and mingw64 has /mingw64/bin first in the path. Packages can be installed for msys2 or mingw32 or mingw64. If in one of the mingw shells and a mingw-specific version of a program is not installed then the msys version which is next in the path will be used. For example if I have the msys2 curl package installed but not the mingw64 curl package then when I run curl in mingw64 it will run the msys2 version.

Then I followed the advice in their wiki to build the packages on my own.

I've built and installed the mingw-w64 version of curl but removed the patch that reportedly fixes the issue. Without the patch everything is fine in the curl tool.

So next I looked at git. I installed the msys2 git package but that uses .netrc. Indeed the issue is present there but that's to be expected. msys2 targets are built with __CYGWIN__ so my patch covers that. But I've hit a roadblock because I can't get the mingw git package working. I get this:

$ git clone https://github.com/bagder/curl.git
Cloning into 'curl'...
ssh: Could not resolve hostname https: Name or service not known
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

So I tried building it from source and that had the same problem. I tried building the git nightly from source but that won't build. They don't have time to support mingw git package from what I was told.

So the mystery remains how @orgads git loads _netrc. Which git are you using?

I think though I have a good idea now what caused this issue. I looked at msysgit's source (what mingw git package is made from) and I can see in the version they use msysgit 1.9.4.1 the default file mode is set to binary:

_CRT_fmode = _O_BINARY

And basically the same thing eventually made its way upstream since then. In the latest git 2.4.2 the default for all fmode is binary as well.

So that seems to me likely to be the issue, though I still prefer to reproduce to be certain. And it's an interesting issue, I think. What obligation do we have as a shared library if the application overrides the default open mode and changes it to binary? Is the onus on the application or on libcurl? If it's libcurl the remedy for this seems to be change each fopen call where we need to read/write text to explicitly specify t for WIN32.

[jay]

I was able to reproduce what I think is happening by modifying simple.c and adding #include <fcntl.h> and in main adding _fmode = _O_BINARY; right after variable declaration, and load netrc curl_easy_setopt(curl, CURLOPT_NETRC, CURL_NETRC_REQUIRED); after curl_easy_init. I linked against the mingw-w64 64-bit version of libcurl:
x86_64-w64-mingw32-gcc -o test_netrc test_netrc.c -lcurl
The password that was sent to the server contained the carriage return.

mingw-w64 does not support _CRT_fmode. Adding it will not change the default file open mode. To confirm I checked their source and can't find it. It looks like that has been known for a while. This is relevant because it implies only msysgit 1.9.4 built using original mingw --and not the later mingw-w64 used by msys2-- would have this issue. So I think it's likely Orgad was using a git built with original mingw and not a git package provided by msys2 which would have used mingw-w64. Or maybe some source other than msysgit. Edit: msysgit 1.9.4 also sets _fmode to binary so the crossed out sentence above is incorrect. And Orgad replied below he's using git-for-windows which is forked from msysgit.

I'm satisfied that the cause is an application setting the default file mode to binary. I'm ready to move on to a solution but I could really use some input.

[orgads]

I'm using git-for-windows.

It does set _fmode to _O_BINARY as you noticed.

What input do you miss?

[jay]

What input do you miss?

Thanks for the follow up. I need input from other people involved in the project. We could handle this by explicitly specifying t every fopen call we want to read lines of text. The problem with doing that is we can't be sure t won't cause an fopen that doesn't recognize it to fail. Most will ignore it as far as I know, but I don't know if all will. Do any fail? We could test fopen with t in configure I guess.

Opening with t regardless already appears once in the source code for the curl tool but I don't know if it should. To work around the issue there is what I committed earlier which is basically an ifdef platform fopen(file, "rt") else fopen file,"r") #endif.

I don't want to pepper the libcurl source with ifdefs every time an fopen is needed to read text. I don't know if everyone is going to want to adhere to putting "t" every time we need to read text, either. What I could instead do is replace fopen with a function or a function-like macro.

#if defined(WIN32) || defined(__CYGWIN__) || defined(SOMETHING_ELSE_THAT_NEEDS_T)
#undef fopen
#define fopen rpl_win_fopen
#endif
and then
rpl_win_fopen {
if(!strstr(mode,"b")) {
// if we're not explicitly binary append a 't' for windows users in case the application switched the default mode.
}
}

Others may have better ideas.

[bagder]

@jay, here's my suggestion on how we could proceed with the code:

#ifdef [the t-needing platforms]
#define FOPEN_READTEXT "rt"
#define FOPEN_WRITETEXT "wt"
#else
#define FOPEN_READTEXT "r"
#define FOPEN_WRITETEXT "w"
#endif

... and then we have all fopens in the code base that read text files done like this (seems to be 9 places in src/ and lib/ right now):

fopen(filename, FOPEN_READTEXT);

... and the version that writes text like this (I believe 4 places):

fopen(filename, FOPEN_WRITETEXT);

[jay]

@bagder Ok I made those changes in e8423f9 but I left alone the mode settings in the vms wrappers that use fopen. Although this is only a libcurl issue as far as I know I made the changes in the curl tool as well.

I think having to specify FOPEN_READTEXT , FOPEN_WRITETEXT may be easy to forget. It may need to be enforced or a warning somewhere (CONTRIBUTE?) so developers know it's required now to explicitly read/write text.

[bagder]

Yes it is certainly easy to forget/miss. It would be nice to write a test for it. One (crude) way would be to just scan for fopen() and "r" in the source, but perhaps we can come up with a clever way to abuse the memdebug fopen() wrapper for it?

[jay]

Do you mean scan like add a regex in checksrc? There would have to be exclusions for the vms wrappers. I don't know how to leverage the fopen wrapper in memdebug for this.

[bagder]

Doing it with checksrc is probably what makes the most sense, and it already has a whitelisting concept so it should work fine. I'll work on it!

[bagder]

Merged in commit 33ee411. I think we can consider this issue done now!

[gvanem]

@bagder wrote:

> #ifdef [the t-needing platforms]
> #define FOPEN_READTEXT "rt"
> #define FOPEN_WRITETEXT "wt"

I saw the commit for this in curl_setup.h. Isn't MSDOS also in the the t-needing platforms ?

[jay]

@gvanem Changed in 5943250. I can't find documentation for it but a simple program in Turbo C that calls fopen("file", "rt") works. I don't build libcurl for MS-DOS but if you do and you could test this change that would be great.