Bug #5714: Unexpected error of STDIN#read with non-ascii input on Windows XP

• Status: Closed
• Assignee: usa (Usaku NAKAMURA)
• Target version: -
• ruby -v: -

[phasis68 (Heesob Park)]

When the input contains non-ascii character, STDIN#read raised Permission denied or Invalid argument error with MSVC compiled version on Windows XP.

C:\work>ruby -ve 'p STDIN.read(5)'
ruby 2.0.0dev (2011-12-05 trunk 33955) [i386-mswin32_100]
한글abcd
-e:1:in `read': Permission denied - <STDIN> (Errno::EACCES)
	from -e:1:in `<main>'

C:\>irb
irb(main):001:0> STDIN.read(5)
한글abcd
Errno::EINVAL: Invalid argument - <STDIN>
	from (irb):1:in `read'
	from (irb):1
	from c:/usr/bin/irb.bat:19:in `<main>'

If the input is ascii only, STDIN.read works fine.

C:\work>ruby -ve 'p STDIN.read(5)'
ruby 2.0.0dev (2011-12-05 trunk 33955) [i386-mswin32_100]
abcdefg
"abcde"

C:\>irb
irb(main):001:0> STDIN.read(5)
abcdefg
=> "abcde"

It is odd but the Mingw compiled version works fine.

C:\work>ruby -ve 'p STDIN.read(5)'
ruby 2.0.0dev (2011-12-05 trunk 33955) [i386-mingw32]
한글abcde
"\xC7\xD1\xB1\xDBabc"

And Ruby 1.9.3p0 works fine.

C:\>ruby -ve 'p STDIN.read(5)'
ruby 1.9.3p0 (2011-10-30 revision 33570) [i386-mswin32_100]
한글abcd
"\xC7\xD1\xB1\xDBabc"

[phasis68 (Heesob Park)]

I guess this issue is due to a bug of _read function of Microsoft Runtime library.

Here is a patch for workaround.

diff --git a/win32.c b/win32.c.new
index 67a392e..4d5a253 100644
--- a/win32.c
+++ b/win32.c.new
@@ -5451,7 +5451,8 @@ rb_w32_read(int fd, void *buf, size_t size)
return -1;
}

• if (_osfile(fd) & FTEXT) {

• isconsole = is_console(_osfhnd(fd));
• if (!isconsole && (_osfile(fd) & FTEXT)) {
  return _read(fd, buf, size);
  }

@@ -5464,7 +5465,6 @@ rb_w32_read(int fd, void *buf, size_t size)
}

 ret = 0;

• isconsole = is_console(_osfhnd(fd));
  if (isconsole) {
  DWORD mode;
  GetConsoleMode((HANDLE)_osfhnd(fd),&mode);

[usa (Usaku NAKAMURA)]

• ruby -v changed from ruby 2.0.0dev (2011-12-05 trunk 33955) [i386-mswin32_100] to -

Hello,

In message "[ruby-core:41500] [ruby-trunk - Bug #5714] Unexpected error of STDIN#read with non-ascii input on Windows XP"
on Dec.06,2011 15:57:36, [e-mail] wrote:

I guess this issue is due to a bug of _read function of Microsoft Runtime library.

agree. (To say strictly, Win32 API has a bug and MSVC runtime is not
coping with it.)

Here is a patch for workaround.

This patch may cause newline code problem.

On my idea, for example, throw away XP :)

Regards,

U.Nakamura [e-mail]

[luislavena (Luis Lavena)]

On Tue, Dec 6, 2011 at 4:18 AM, U.Nakamura [e-mail] wrote:

On my idea, for example, throw away XP :)

_WIN32_WINNT

[nobu (Nobuyoshi Nakada)]

Hi,

(11/12/06 22:34), Luis Lavena wrote:

BTW, I'm collecting stats about Ruby-users with Windows to determine
the best option for that, can you spread the word?

https://docs.google.com/spreadsheet/viewform?hl=en_US&formkey=dGp1VGthak50dDZSYzZtTG55c29scFE6MQ#gid=0

Why isn't ruby 2.0.0 there? :)

--
Nobu Nakada

[h.shirosaki (Hiroshi Shirosaki)]

•    if (_osfile(fd) & FTEXT) {

•    isconsole

[phasis68 (Heesob Park)]

Hi,

2011/12/7 Hiroshi Shirosaki [e-mail]

•    if (_osfile(fd) & FTEXT) {

•    isconsole = is_console(_osfhnd(fd));
•    if (!isconsole && (_osfile(fd) & FTEXT)) {
         return _read(fd, buf, size);
      }

I intended STDIN doesn't use text mode at #5562 patch. So STDIN
doesn't need to use _read().
I thought (_osfile(fd) & FTEXT) should be false in the case of STDIN.

I found that the initial value of _osfile(STDIN) is 0xC1 and same for
_osfile(STDOUT) and _osfile(STDERR).
The value 0xC1 means (FTEXT | FDEV | FOPEN).

Here is another patch for

diff --git a/win32.c b/win32.c.new
index 67a392e..db9e222 100644
--- a/win32.c
+++ b/win32.c.new
@@ -2259,12 +2259,21 @@ init_stdhandle(void)
if (fileno(stdin) < 0) {
stdin->_file = open_null(0);
}

• else {
• setmode(fileno(stdin), O_BINARY); // _osfile(fileno(stdin)) &= ~FTEXT;
• }
  if (fileno(stdout) < 0) {
  stdout->_file = open_null(1);
  }
• else {
• setmode(fileno(stdout), O_BINARY); // _osfile(fileno(stdout)) &= ~FTEXT;
• }
  if (fileno(stderr) < 0) {
  stderr->_file = open_null(2);
  }
• else {
• setmode(fileno(stderr), O_BINARY); // _osfile(fileno(stderr)) &= ~FTEXT;
• }
  if (nullfd >= 0 && !keep) close(nullfd);
  setvbuf(stderr, NULL, _IONBF, 0);
  }

Regards,
Park Heesob

[h.shirosaki (Hiroshi Shirosaki)]

• File read_binmode.patch read_binmode.patch added

It seems read with length should always be binary mode. #5562 patch lacks this point.
I think this patch fixes above issue.

It seems OK that stdin is default binary mode, but if stdout and stderr are default binary mode, newline conversion breaks.
We can confirm this by the following tests.

make test-all TESTS="ruby/test_io_m17n.rb"

[luislavena (Luis Lavena)]

• Category set to build
• Assignee set to usa (Usaku NAKAMURA)

Usa, what do you think about attached read_binmode.patch?

[h.shirosaki (Hiroshi Shirosaki)]

• File read_binmode2.patch read_binmode2.patch added

I modified the patch because rubygems didn't work properly.
However, I think it is consistent with _read() api behavior that read(length) does CRLF conversion.
Ruby 1.9 doesn't do CRLF conversion, but ruby 1.8 does.

ruby -ve "open('t1', 'wb') {|f| f.write("a\r\nb")}; open ('t1', 'r') {|f| p f.read(4)}"
ruby 1.8.7 (2011-06-30 patchlevel 352) [i386-mingw32]
"a\nb"

ruby -ve "open('t1', 'wb') {|f| f.write("a\r\nb")}; open ('t1', 'r') {|f| p f.read(4)}"
ruby 1.9.2p290 (2011-07-09) [i386-mingw32]
"a\r\nb"

ruby -ve "open('t1', 'wb') {|f| f.write("a\r\nb")}; open ('t1', 'r') {|f| p f.read(4)}"
ruby 1.9.3p0 (2011-10-30) [i386-mingw32]
"a\r\nb"

[usa (Usaku NAKAMURA)]

Hello,

In message "[ruby-core:41578] [ruby-trunk - Bug #5714] Unexpected error of STDIN#read with non-ascii input on Windows XP"
on Dec.10,2011 06:49:55, [e-mail] wrote:

Category set to build

build?

Usa, what do you think about attached read_binmode.patch?

Difficult, difficult...

Of course, as Shirosaki-san says, IO#read with length should always
be binary mode.
But if setting binmode once, it's never canceled.
So, I doubt that such implicit mode setting in IO#read is right.

Regards,

U.Nakamura [e-mail]

[luislavena (Luis Lavena)]

On Sun, Dec 11, 2011 at 11:42 PM, U.Nakamura [e-mail] wrote:

Hello,

In message "[ruby-core:41578] [ruby-trunk - Bug #5714] Unexpected error of STDIN#read with non-ascii input on Windows XP"
   on Dec.10,2011 06:49:55, [e-mail] wrote:

Category set to build

build?

Sorry, meant to set core, but we don't have any category to describe
platform-specific issues.

Usa, what do you think about attached read_binmode.patch?

Difficult, difficult...

Of course, as Shirosaki-san says, IO#read with length should always
be binary mode.
But if setting binmode once, it's never canceled.
So, I doubt that such implicit mode setting in IO#read is right.

Agree.

Hiroshi and Heesob, do you guys think can solve this without reverting
the changes of #5562?

If not, then reverting seems the only alternative.

Luis Lavena
AREA 17

Perfection in design is achieved not when there is nothing more to add,
but rather when there is nothing more to take away.
Antoine de Saint-Exupéry

[h.shirosaki (Hiroshi Shirosaki)]

• File read_binmode_fix_r34024.patch read_binmode_fix_r34024.patch added

Hiroshi and Heesob, do you guys think can solve this without reverting
the changes of #5562?

It's difficult, but it seems switching from binary mode to text mode would be possible. I added binary and text mixed test cases.
I use implicit mode setting at other positions.

File cursor seeking with setting binary mode had a bug. I fixed it to take care of '\r'.
Is there other test cases to check?

[usa (Usaku NAKAMURA)]

Hello,

In message "[ruby-core:41628] [ruby-trunk - Bug #5714] Unexpected error of STDIN#read with non-ascii input on Windows XP"
on Dec.13,2011 16:44:29, [e-mail] wrote:

It's difficult, but it seems switching from binary mode to text mode would be possible. I added binary and text mixed test cases.
I use implicit mode setting at other positions.

File cursor seeking with setting binary mode had a bug. I fixed it to take care of '\r'.
Is there other test cases to check?

Although I don't want to trouble you...

str = "a\nb"
generate_file("tmp", str)
open("tmp", "r") do |f|
assert_equal(str, f.read(3))
end

Regards,

U.Nakamura [e-mail]

[h.shirosaki (Hiroshi Shirosaki)]

• File read_binmode_fix_r34035.patch read_binmode_fix_r34035.patch added

str = "a\nb"
generate_file("tmp", str)
open("tmp", "r") do |f|
assert_equal(str, f.read(3))
end

Thank you. I added above test and some other tests.
I added one more mode setting for safe.

"make test" passed.
"make test-all" failures and errors were same as before patch.

Could you check this patch?

[usa (Usaku NAKAMURA)]

Hello,

In message "[ruby-core:41641] [ruby-trunk - Bug #5714] Unexpected error of STDIN#read with non-ascii input on Windows XP"
on Dec.14,2011 17:04:16, [e-mail] wrote:

Thank you. I added above test and some other tests.
I added one more mode setting for safe.

"make test" passed.
"make test-all" failures and errors were same as before patch.

Great!
I've also retested and get same results.

Could you check this patch?

I've checked in your patches at r34043.
Thank you!

Regards,

U.Nakamura [e-mail]

[h.shirosaki (Hiroshi Shirosaki)]

• File set_binmode_fix.patch set_binmode_fix.patch added

Hi Usaku NAKAMURA san,

I've checked in your patches at r34043.

Thank you for check and merge, and test fix.

This is a follow up fix.
Sorry for my mistake. I noticed an issue that file cursor seek does't work properly.
In the case, trying to seek cursor before the beginning of the file and it causes infinite loop.
I added the test case.

I've fixed this and refined error handling.

I checked "make test" passed and "make test-all" Errors/Failures were same as before patch.

[usa (Usaku NAKAMURA)]

Hello,

In message "[ruby-core:41671] [ruby-trunk - Bug #5714] Unexpected error of STDIN#read with non-ascii input on Windows XP"
on Dec.15,2011 14:58:07, [e-mail] wrote:

Thank you for check and merge, and test fix.

Your welcome :)

This is a follow up fix.
Sorry for my mistake. I noticed an issue that file cursor seek does't work properly.
In the case, trying to seek cursor before the beginning of the file and it causes infinite loop.
I added the test case.

I've fixed this and refined error handling.

I checked "make test" passed and "make test-all" Errors/Failures were same as before patch.

I think you should check in your patches by yourself, if you
are not disagreeable.
Do you want to do so?

Regards,

U.Nakamura [e-mail]

[h.shirosaki (Hiroshi Shirosaki)]

Hello,

I think you should check in your patches by yourself, if you
are not disagreeable.
Do you want to do so?

Yes. I'll check in that if that's acceptable.

Thanks,

Hiroshi Shirosaki

[usa (Usaku NAKAMURA)]

• Status changed from Open to Closed
• % Done changed from 0 to 100

This issue was solved with changeset r34043.
Heesob, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.

---

• win32/win32.c, include/ruby/win32.h (rb_w32_fd_is_text): new function.

• win32/win32.c (init_stdhandle): set default mode of stdin as binmode.

• io.c (set_binary_mode_with_seek_cur): new function to replace
  SET_BINARY_MODE_WITH_SEEK_CUR macro. now returns previous mode of the
  fd and take care of LF in rbuf.

• io.c (do_writeconv): set text mode when needed.

• io.c (io_read): need to change the mode of the IO to binmode
  temporally when the length for IO#read, because IO#read with length
  must behave so.

• test/ruby/test_io_m17n.rb (TestIO_M17N#est_{read_with_length,
  read_with_length_binmode,get[cs]_and_read_with_binmode,
  read_with_binmode_and_get[cs],read_write_with_binmode}): tests for
  above changes.

all patches are written by Hiroshi Shirosaki. [ruby-core:41496]
[Feature #5714]

[usa (Usaku NAKAMURA)]

Hello,

In message "[ruby-core:41689] Re: [ruby-trunk - Bug #5714] Unexpected error of STDIN#read with non-ascii input on Windows XP"
on Dec.16,2011 14:07:18, [e-mail] wrote:

I think you should check in your patches by yourself, if you
are not disagreeable.
Do you want to do so?

Yes. I'll check in that if that's acceptable.

So, send your SSH public key to cvs-admin at ruby-lang.org,
and wait the response.

Shugo-san, yoroshiku.

Regards,

U.Nakamura [e-mail]

[h.shirosaki (Hiroshi Shirosaki)]

• File set_binmode_fix_r34120.patch set_binmode_fix_r34120.patch added

I updated the patch while waiting for the response.

Cleanups

• remove unnecessary parentheses of fptr
• use return value of setmode() which returns the previous translation mode if successful
  http://msdn.microsoft.com/en-us/library/tw4k6df8.aspx

Test results of trunk r34120 mingw32 on Windows7 64bit

"make test" passed.
test-all failures and errors were as below. Result is same as before patch.

test_race_exception(TestRequire)
test_generate_bin_bindir_with_user_install_warning(TestGemInstaller)
test_s_open_error(TestGDBM)
test_s_open_create_new(TestGDBM)
test_thread_timer_and_interrupt(TestThreadGroup) # This test was hung-up.
test_constants(OpenSSL::TestConfig)
test_reorganize(TestGDBM)
test_filename_as_bytes_extutf8(TestDir_M17N)
test_filename_extutf8_inteucjp_unrepresentable(TestDir_M17N)