This is an archive of the discontinued LLVM Phabricator instance.

Fix raw_fd_ostream::write_impl hang due to an infinite loop with large output
ClosedPublic

Authored by gbreynoo on Jul 4 2018, 10:48 AM.

Details

Summary

On windows when raw_fd_ostream::write_impl calls write, a 32 bit input is required for character count. As a variable with size_t is used for this argument on x64 integral demotion occurs. In the case of large files an infinite loop follows.
See: https://bugs.llvm.org/show_bug.cgi?id=37926

This fix allows the output of files larger than previous int32 limit.

Diff Detail

Repository
rL LLVM

Event Timeline

gbreynoo created this revision.Jul 4 2018, 10:48 AM
zturner added inline comments.Jul 10 2018, 10:37 AM
lib/Support/raw_ostream.cpp
617–618 ↗(On Diff #154136)

Doesn't it actually require 32-bit unsigned input?

https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/write

int _write(
   int fd,
   const void *buffer,
   unsigned int count
);
gbreynoo added inline comments.Jul 11 2018, 8:18 AM
lib/Support/raw_ostream.cpp
617–618 ↗(On Diff #154136)

I'll clarify this in the comment above.
Although the input type for _write is unsigned, the return type is signed ("-1" is returned in cases of an error). This is checked for in line 642. As the standard return of _write is to return how many bytes are written, in the case this value is larger than the signed max, the return value is a negative value. If the maximum unsigned value is written this will be -1 and indicate an error.

As the comment below states, windows _write is following POSIX as the behaviour for a write greater than SSIZE_MAX is implementation defined. I think it's best to keep to the portable behaviour below and not write anything windows specific to allow for a larger than SSIZE_MAX write.

Ok, makes sense. Lgtm then

gbreynoo updated this revision to Diff 155017.Jul 11 2018, 8:56 AM
ruiu added a comment.Jul 11 2018, 9:04 AM

Do we have to save the number of system calls if we have really huge (>2GiB) buffer? I'm thinking if we can always use 1GiB as the maximum write size. That should make code simpler and easy to maintain.

gbreynoo updated this revision to Diff 156057.Jul 18 2018, 6:41 AM

Made maximum write size portable as suggested by Rui

I think you addressed all the concerns and i lgtmed earlier anyway. Go for
it :) if there’s anything else you can follow up with another patch

This revision was not accepted when it landed; it landed in state Needs Review.Jul 26 2018, 6:22 AM
This revision was automatically updated to reflect the committed changes.