407844 - js.c: missing last character when using readline() on files

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[balthasur]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.11) Gecko/20071127 Firefox/2.0.0.11
Build Identifier: http://ftp.mozilla.org/pub/mozilla.org/js/js-1.60.tar.gz

when using readline() in js.c (javascript shell) to read a file which:
1. not ended with a newline character
2. only has one character in the last line of the file (the last character
the readline() will not return the last character of the file

Reproducible: Always

Steps to Reproduce:
1. consider following scripts in a file named echo.js (the last character of the file is '}':

while (s = readline()) { print(s);
}<eof>

2. run the script: js echo.js < echo.js
Actual Results:  
the output will be (the '}' was missing):

while (s = readline()) { print(s);


Expected Results:  
the expected output should be:

while (s = readline()) { print(s);
}


the code in ReadLine():

str = JS_NewString(cx, buf, buflength - 1);

may be modified as follows:

str = JS_NewString(cx, buf, (buf[buflength - 1] == '\0') ? buflength - 1 : buflength);

since the last characters may not always be double '\0' when reaching eof...

Comment 1

[Brian Crowder]

Are you able to reproduce this on a build of spidermonkey from trunk CVS?

Comment 2

[balthasur]

Yes, the bug is still alive in the trunk CVS, the buf[buflength - 1] is not always equal to '\0' when reach eof, and buflength - 1 is not always the actual length of the string.

But the bug seemed disappeared when I build the trunk CVS on linux (I don't know why...)

Comment 3

[Brian Crowder]

Yes, I can't reproduce this bug on Mac OS X, either.  Must be Windows-only?  I'll try windows again when I get a chance.

Comment 4

[Jeff Walden [:Waldo]]

I also see this in a trunk build on Linux, so it's not Windows-only.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Easy fix

This also includes a trivial optimization to avoid growing the buffer if we got a short read.

This is why everybody should end all of their files with a newline!

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Oops

Oops, the last patch's "short read" check was off by two(!)

Comment 7

[Brian Crowder]

Comment on attachment 293776 [details] [diff] [review]
Oops

     buf = (char *) JS_malloc(cx, bufsize);
+    sawNewline = JS_FALSE;
     if (!buf)
         return JS_FALSE;

Move the sawNewLine init to after the if (!buf) and return.

+        } else if (buflength < bufsize - 1) {
+            printf("%d %d\n", buflength, bufsize);

What're we doing here?  Needs a comment at least, or maybe this is leftover debug code?

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Yeah, the printf is leftover debug code -- I forgot to rediff after I took it out.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk with implicit a=brendan for NPOTB js.c

Comment 10

[Bob Clary [:bc] (inactive)]

Although it is possible to write a js test for this, I'm not inclined to do so unless someone else thinks it is worth while. Change the in-testsuite- to a ? if you want a test added.