1278523 - Add shell helper function that emulates readline() behavior on string buffer

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Christian Holler (:decoder)]

Attachment: jsshell-readlinebuf.patch

The LangFuzz driver used for JS shell fuzzing uses a while loop combined with readline() to read code from stdin. All inputs are stored in a log by the fuzzer, so the original test can be replayed by re-running the driver and piping the log into it.

For reduction and single-file testcase creation, the log is first reduced and then either it reproduces on its own without the driver, or it has to be inlined into the driver as a template string and then processed by the driver line by line. This is typically done by calling .split("\n") on the string and then using .shift() on the result, e.g.:


> var lfLog = `some multi-line fuzz code here`
> lfLog = lfLog.split("\n");
> while(true) {
>  line = lfLog.shift(); // This used to be line = readline();
>  ...
>}

The problem here is: I had multiple testcases where any such conversion failed (for unknown reasons). I am right now debugging an intermittent GC crash that only reproduces with the driver using readline() and no other method worked for conversion.

So what I did is, I wrote a readlineBuf() method that you initially pass a string object and then subsequent calls to it return it line by line just like readline() does. In fact, with this function, I was able to create a single-file testcase for that GC crash.

The patch is attached and I'd be glad to receive feedback on it. Also, if you have another idea how to make this conversion without a new helper function, let me know and I will try it out!

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8760703 [details] [diff] [review]
jsshell-readlinebuf.patch

Review of attachment 8760703 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I think this is fine to have if this helps reproducing fuzzbugs. For the memory handling of strings, I'll defer to jandem as he knows better the string copy / encoding features.

::: js/src/shell/js.cpp
@@ +181,5 @@
>      int exitCode;
>      bool quitting;
> +
> +    UniquePtr<char> readLineBuf;
> +    size_t readLineBufPos;

You could even use statics (this is done in several places in this file).

@@ +329,5 @@
>      promiseRejectionTrackerCallback(rt, NullValue()),
>  #endif // SPIDERMONKEY_PROMISE
>      exitCode(0),
> +    quitting(false),
> +    readLineBuf(),

nit: no need to put this line (that's the default if there's no initializer in the initializer list)

@@ +1742,5 @@
> +
> +        size_t buflen = strlen(currentBuf);
> +        size_t len = 0;
> +
> +        if (!buflen)  {

nit: only one space after )

@@ +1758,5 @@
> +        JSString* str = JS_NewStringCopyN(cx, currentBuf, len);
> +        if (!str)
> +            return false;
> +
> +        sr->readLineBufPos += len + 1;

Doesn't this mean that the next call to ReadLineBuf will read out of the buffer's bounds, when we reached the last line of the buffer? Sounds dangerous...

@@ +1765,5 @@
> +        return true;
> +    } else if (args.length() == 1) {
> +        if (sr->readLineBuf) {
> +            /* Free existing buffer and set it to NULL until we have a new one */
> +            // XXX: Do I need to free, or does it auto-delete when assigned a new ptr?

UniquePtr does the free for you. Actually, you don't need even need this code: I think resetting the UniquePtr will also delete the former value for you.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8760703 [details] [diff] [review]
jsshell-readlinebuf.patch

Review of attachment 8760703 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Just some nits.

::: js/src/shell/js.cpp
@@ +180,5 @@
>  
>      int exitCode;
>      bool quitting;
> +
> +    UniquePtr<char> readLineBuf;

Nit: can also use UniqueChars here and below I think.

@@ +1740,5 @@
> +
> +        char* currentBuf = sr->readLineBuf.get() + sr->readLineBufPos;
> +
> +        size_t buflen = strlen(currentBuf);
> +        size_t len = 0;

Nit: move this right before the while loop that uses it.

@@ +1743,5 @@
> +        size_t buflen = strlen(currentBuf);
> +        size_t len = 0;
> +
> +        if (!buflen)  {
> +            args.rval().set(NullValue());

Nit: .setNull()

@@ +1748,5 @@
> +            return true;
> +        }
> +
> +        while(len < buflen) {
> +            if (currentBuf[len] == '\n') {

Nit: no {}

@@ +1762,5 @@
> +        sr->readLineBufPos += len + 1;
> +
> +        args.rval().setString(str);
> +        return true;
> +    } else if (args.length() == 1) {

Nit: no else after return, and below.

@@ +1767,5 @@
> +        if (sr->readLineBuf) {
> +            /* Free existing buffer and set it to NULL until we have a new one */
> +            // XXX: Do I need to free, or does it auto-delete when assigned a new ptr?
> +            //JS_free(cx, sr->readLineBuf);
> +            sr->readLineBuf = nullptr;

Assigning nullptr will free it, you can do this a bit more explicitly with:

sr->readLineBuf.reset();

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> You could even use statics (this is done in several places in this file).

That can race (due to evalInWorker).

Comment 4

[Christian Holler (:decoder)]

Attachment: Bug 1278523 - Implement jsshell helper function readlineBuf.

Review commit: https://reviewboard.mozilla.org/r/58210/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58210/

Comment 5

[Christian Holler (:decoder)]

Comment on attachment 8760751 [details]
Bug 1278523 - Implement jsshell helper function readlineBuf.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58210/diff/1-2/

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8760751 [details]
Bug 1278523 - Implement jsshell helper function readlineBuf.

Clearing NI because of the issue we discussed. We should also add a (jit-)test for some interesting cases (string with/without trailing \n, etc).

Comment 7

[Christian Holler (:decoder)]

Comment on attachment 8760751 [details]
Bug 1278523 - Implement jsshell helper function readlineBuf.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58210/diff/2-3/

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8760751 [details]
Bug 1278523 - Implement jsshell helper function readlineBuf.

https://reviewboard.mozilla.org/r/58210/#review55084

Thanks for adding the tests!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b4071e76539
Implement jsshell helper function readlineBuf. r=jandem

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8b4071e76539