Bug 129114 – jsc-cli needs a basic story for text IO

• Status: NEW
• Component: JavaScriptCore

[Mark Hahnenberg]

A simple solution that would get us pretty far would be to offer an IO module that was a higher-level wrapper around file-descriptor-based open/read/write/close OS level APIs.

[Mark Hahnenberg]

Created attachment 225008 [details]
Patch

[Mark Hahnenberg]

Comment on attachment 225008 [details]
Patch

Actually, this patch still has issues.

[Mark Hahnenberg]

Created attachment 225261 [details]
Patch

[Oliver Hunt]

Comment on attachment 225261 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=225261&action=review

How does this io performance compare to other scripting environments - it should be as fast or faster.

r- due to buffer overflow

> Tools/jsc-cli/jsc-cli/lib/IOModule.m:102
> +        char* buffer = (char*)malloc(sizeof(char) * (bytes + 1));
> +        size_t result = fread(buffer, sizeof(char), bytes, handle.stream);

Hello buffer overflow :)

> Tools/jsc-cli/jsc-cli/lib/IOModule.m:106
> +            setErrorWithErrno();
> +            free(buffer);
> +            return (JSValue *)nil;

Please just throw an error on an io error, don't mess around with separate errors

> Tools/jsc-cli/jsc-cli/lib/IOModule.m:142
> +            setErrorWithErrno();

again exception

> Tools/jsc-cli/jsc-cli/lib/IOModule.m:145
> +        return [[NSString alloc] initWithBytes:line length:length encoding:NSUTF8StringEncoding];

Will this copy the bytes?

> Tools/jsc-cli/jsc-cli/lib/IOModule.m:151
> +            setErrorWithErrno();

exception!

[Oliver Hunt]

Comment on attachment 225261 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=225261&action=review

> Tools/jsc-cli/jsc-cli/lib/IOModule.m:150
> +        int result = fclose(handle.stream);
> +        if (result != 0)

can we clear the stream field here? I don't want any bogus reuse to occur

[Mark Hahnenberg]

(In reply to comment #4)
> (From update of attachment 225261 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225261&action=review
> 
> How does this io performance compare to other scripting environments - it should be as fast or faster.
It's relatively slow due to lots of copying across the API boundary. I'm not sure we could avoid this without new API functionality.

> 
> > Tools/jsc-cli/jsc-cli/lib/IOModule.m:106
> > +            setErrorWithErrno();
> > +            free(buffer);
> > +            return (JSValue *)nil;
> 
> Please just throw an error on an io error, don't mess around with separate errors
I'm not sure what you mean by this. Why should you get the same error if you don't have permission to access the file as when the file doesn't exist? That seems like a pure usability regression.

[Mark Hahnenberg]

(In reply to comment #5)
> (From update of attachment 225261 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225261&action=review
> 
> > Tools/jsc-cli/jsc-cli/lib/IOModule.m:150
> > +        int result = fclose(handle.stream);
> > +        if (result != 0)
> 
> can we clear the stream field here? I don't want any bogus reuse to occur

Will do.

[Mark Hahnenberg]

> > Tools/jsc-cli/jsc-cli/lib/IOModule.m:145
> > +        return [[NSString alloc] initWithBytes:line length:length encoding:NSUTF8StringEncoding];
> 
> Will this copy the bytes?
Yep. - initWithBytesNoCopy:length:encoding: is the version that doesn't copy the bytes.