Fixing asm2wasm crashing on \r when built with MinGW #986

jerstlouis · 2017-04-28T22:55:25Z

Also enlarging stack size for MinGW

kripken · 2017-04-29T20:47:13Z

src/support/file.cpp

@@ -26,7 +26,8 @@ T wasm::read_file(const std::string &filename, Flags::BinaryOption binary, Flags
  if (debug == Flags::Debug) std::cerr << "Loading '" << filename << "'..." << std::endl;
  std::ifstream infile;
  std::ios_base::openmode flags = std::ifstream::in;
-  if (binary == Flags::Binary) flags |= std::ifstream::binary;
+  // asm2wasm.exe built with MinGW would crash with files containing '\r' when opened without the binary flag


does anyone know why mingw is special here?

Is it possible that it's not MinGW specific but also an issue when built with Visual Studio?

I'm not sure, but the reason I am guessing it's mingw-specific is that people have built and used this with vs. At least I think so, I didn't do it myself...

If the code already always handles skipping \r and works on Linux which does not have a non-binary mode, then there's no reason why the file should be opened without the binary flag, and I can't see how it would work better on Visual Studio.

There's other reports of asm2wasm crashing, see for example: #327 which crashes at exactly the same spot.

Why are \r even generated though? Are they only generated when done on Windows? It doesn't make sense to me to generate files meant to be used across platforms differently whether they're created on one platform or another. The 'text' mode and \r\n really needs to disappear anyway, it's such a horrible relic of the past.

I think when we generate the asm.js text, on windows the text-handling python code in windows just emits \rs automatically. @juj knows more.

If there is no danger here, then opening as binary sounds ok, but I don't know enough about windows and text handling there.

Again opening as binary when outputting the asm.js will avoid those \r's. This will be an issue trying to get reproducible output across systems. Really, the 'text' mode has no reason to be.

kripken · 2017-04-29T20:48:14Z

CMakeLists.txt

@@ -148,6 +148,7 @@ ELSE()
  ADD_COMPILE_FLAG("-Wno-unused-parameter")
  ADD_COMPILE_FLAG("-fno-omit-frame-pointer")
  IF(WIN32)
+    ADD_LINK_FLAG("-Wl,--stack,8388608")


please add a comment for this. i assume it's that unix stack sizes are normally 8MB but we need to add this for mingw?

Was just making it the same as the MSVC section. MinGW defaults to 2 MB I think.

kripken · 2017-07-11T21:24:39Z

ping @juj, see question higher up

juj · 2017-07-12T10:27:50Z

Sorry for missing this before!

Before we fix this, we really need to have a test case in Emscripten suite about this, since these types of items are critical and we don't routinely test MinGW compiler at the moment. Currently we run the Binaryen suite on VS2015 on each commit, but that is not catching the issue at the moment. @griffin2000, @jerstlouis: do you know of a short test case to repro to see the crash? I'm now building with Binaryen to test this out.

juj · 2017-07-12T13:15:11Z

Investigated this in closer detail, I see that building Emscripten and Binaryen with MinGW 7.1.0, it does fail e.g. in Emscripten's test other.test_binaryen_debug.

The problem indeed is MinGW specific and does not appear in Visual Studio. However this is due to a bug in function wasm::read_file that only manifests in MinGW libstdc++ and not Visual Studio's c++ library.

The behavior that text file reading on Windows (in both VS and MinGW) has is that if a file has a byte sequence \r\n on disk, then reading those bytes in to memory in text mode should result in \r\n getting converted on the fly to \n in memory. MSDN: "Also, in text mode, carriage return–linefeed combinations are translated into single linefeeds on input, and linefeed characters are translated to carriage return–linefeed combinations on output.".

This has the effect that the size on disk of a file is larger in bytes than the file size in memory, since \r\n -> \n contracts the size by one byte when going to memory. The function ifstream::tellg() used in wasm::read_file always returns the size of the file in disk bytes, and that is the size that is allocated to memory. When the file is later read as text, there are more bytes allocated to it in memory than are needed, since the representation shrinks as \r bytes are removed.

The behavioral difference that is observed here between Visual Studio and MinGW is that on Visual Studio, calling infile.read(&input[0], insize);, where insize is the disk size (and not the memory size), the excess bytes are not touched and they all end up being \0s. This is benign for parser.h, and the code then constructs a std::string that just has excess \0s at the end, one for each removed \r character. However on MinGW it looks that the excess memory ends up getting garbage characters, most likely due to an in-memory contraction algorithm that removes \rs in one go after a binary read. This is the root source of the crash, i.e. wasm::read_file returns a std::string that has garbage characters in the end, which then later fails in parser.h, when it is rolling through the garbage characters.

The bug is in wasm::read_file as opposed to MinGW I believe. One should not expect that infile.read() would read exactly as many characters in as is requested in bytes. However there is no good C++ fstream API to ask how many characters it was able to read. Therefore instead, PR #1088 shows how to fix using C file API, which works on MinGW.

The difference with the fix here and #1088 is that unconditionally passing binary flag to file open means that, on Windows, the \r characters will begin to appear in the in memory representations of files opened in text mode. In parser.h function isSpace, those are currently taken into account, so for the parser at least, that is not a problem. (bool isSpace(char x) { return x == 32 || x == 9 || x == 10 || x == 13; } /* space, tab, linefeed/newline, or return */). However it should be made certain that all of the \r characters are efficiently consumed, since this PR now has an asymmetricity that text files will then be opened in binary mode, but can still be written in text mode, which would mean that if any \r\n sequences are retained in memory, then those would get duplicated to \r\r\n if they are later written out as text. I'm not sure if that can ever happen though.

The PR #1088 retains the current property that text content in memory only ever has \n newlines and never \r\n, though it does revert to C API to be able to do that, since fstreams aren't too flexible for this purpose. Ultimately, I am ambivalent as to which way to go to fix. This PR is ok if we know that \rs in memory are always ok and expected (are there locations in code beyond parser.h that would need to be audited for this?), and that it will not cause incorrect \r\r\n expansion later due to this asymmetric nature.

juj · 2017-07-12T13:17:13Z

does anyone know why mingw is special here?

The difference seems to be as to how VS and MinGW perform the contraction of \r characters. VS seems to discard \r characters as it is going through the read, whereas MinGW seems to first read the whole file in memory in binary, and then use an in-memory filter algorithm to weed out \rs, which resulted in garbage chars (leftovers of the input file) being observed as opposed to \0s.

juj · 2017-07-12T13:19:03Z

Why are \r even generated though? Are they only generated when done on Windows?

Emscripten has a linker flag --output_eol linux|windows that can be used to choose the desired output line ending mode, on both Windows and Linux hosts. It defaults to the host choice, so on Windows, you can pass --output_eol linux linker flag to get \n line endings.

jerstlouis · 2017-07-12T14:03:09Z

http://www.cplusplus.com/reference/istream/istream/read/

"The number of characters successfully read and stored by this function can be accessed by calling member gcount."

jerstlouis · 2017-07-12T14:06:00Z

My approach to parsing text files that may or may not contain \r is always to ignore \r. Simpler that way. I would recommend doing the same and avoiding all these "text" mode caveats. Through I think gcount should still be used here.

juj · 2017-07-17T17:07:12Z

Rewrote the PR to use gcount.

kripken · 2017-07-18T16:42:09Z

I believe this has been fixed by #1088, thanks everyone.

juj · 2017-07-19T19:19:06Z

There was that unrelated change in this PR, added #1103 for that.

jerstlouis added 2 commits April 28, 2017 18:37

Fixed asm2wasm crashing on files containing '\r' when built with MinGW

05ea94e

Increasing stack when built with MinGW

ad20584

jerstlouis force-pushed the master branch from f54bd20 to ad20584 Compare April 28, 2017 23:05

kripken reviewed Apr 29, 2017

View reviewed changes

juj mentioned this pull request Jul 17, 2017

fix_text_file_reads #1088

Merged

kripken closed this Jul 18, 2017

juj mentioned this pull request Jul 19, 2017

mingw_match_visualstudio_stack #1103

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing asm2wasm crashing on \r when built with MinGW #986

Fixing asm2wasm crashing on \r when built with MinGW #986

jerstlouis commented Apr 28, 2017

kripken Apr 29, 2017

jerstlouis Apr 30, 2017

kripken May 1, 2017

jerstlouis May 1, 2017

jerstlouis May 1, 2017

kripken May 3, 2017 •

edited

Loading

jerstlouis May 3, 2017

kripken Apr 29, 2017

jerstlouis Apr 29, 2017 •

edited

Loading

kripken commented Jul 11, 2017

juj commented Jul 12, 2017

juj commented Jul 12, 2017 •

edited

Loading

juj commented Jul 12, 2017

juj commented Jul 12, 2017

jerstlouis commented Jul 12, 2017

jerstlouis commented Jul 12, 2017

juj commented Jul 17, 2017

kripken commented Jul 18, 2017

juj commented Jul 19, 2017

Fixing asm2wasm crashing on \r when built with MinGW #986

Fixing asm2wasm crashing on \r when built with MinGW #986

Conversation

jerstlouis commented Apr 28, 2017

kripken Apr 29, 2017

Choose a reason for hiding this comment

jerstlouis Apr 30, 2017

Choose a reason for hiding this comment

kripken May 1, 2017

Choose a reason for hiding this comment

jerstlouis May 1, 2017

Choose a reason for hiding this comment

jerstlouis May 1, 2017

Choose a reason for hiding this comment

kripken May 3, 2017 • edited Loading

Choose a reason for hiding this comment

jerstlouis May 3, 2017

Choose a reason for hiding this comment

kripken Apr 29, 2017

Choose a reason for hiding this comment

jerstlouis Apr 29, 2017 • edited Loading

Choose a reason for hiding this comment

kripken commented Jul 11, 2017

juj commented Jul 12, 2017

juj commented Jul 12, 2017 • edited Loading

juj commented Jul 12, 2017

juj commented Jul 12, 2017

jerstlouis commented Jul 12, 2017

jerstlouis commented Jul 12, 2017

juj commented Jul 17, 2017

kripken commented Jul 18, 2017

juj commented Jul 19, 2017

kripken May 3, 2017 •

edited

Loading

jerstlouis Apr 29, 2017 •

edited

Loading

juj commented Jul 12, 2017 •

edited

Loading