Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not panic in ‘os’ module if argv.Length() is 0 #1117

Merged

Conversation

samhocevar
Copy link
Contributor

No description provided.

@flimzy
Copy link
Member

flimzy commented May 9, 2022

Hi, @samhocevar , thanks for the contribution. To help understand the implications, can you explain when you ran into argv having a length of 0?

@samhocevar
Copy link
Contributor Author

samhocevar commented May 9, 2022

Sure; I built the following Go program:

func main() {
    fmt.Printf("%v", len(os.Args))
}

I am using GopherJS 1.17.2+go1.17.9 on the Windows platform, using: gopherjs build ./src.

I tweaked the GopherJS-generated src.js so that it declares a my_code() function, which lets me first initialise BrowserFS (the version from npm), and then call that main function:

<script src="node_modules/browserfs/dist/browserfs.js"></script>
<script src="src.js"></script>
<script type="text/javascript">
  BrowserFS.install(window);
  BrowserFS.configure({ fs: "XmlHttpRequest", options: { index: "index.json" }, }, function(e) { if (e) { throw e; }
    window.fs = BrowserFS.BFSRequire('fs');
    window.fs.constants = { O_WRONLY: 1, O_RDWR: 2, O_CREAT: 64, O_TRUNC: 512, O_APPEND: 1024, O_EXCL: 128 };
    my_code();
  });
</script>

The stacktrace looks like this:

src.js:1561 Uncaught Error: runtime error: makeslice: len out of range
    at $callDeferred (src.js:1561:17)
    at $panic (src.js:1616:3)
    at throw$1 (gopherjs__runtime.go:432:3)
    at $makeSlice (src.js:1399:5)
    at init$1 (gopherjs__os.go:22:5)
    at Object.$init (src.js:36392:3)
    at Object.$init (src.js:40487:11)
    at Object.$init (src.js:46644:12)
    at $init (src.js:270206:12)
    at $goroutine (src.js:1636:19)
    at $runScheduled (src.js:1682:7)
    at $schedule (src.js:1706:5)
    at $go (src.js:1668:3)
    at src.js:270285:1
    at src.js:270288:4

@nevkontakte
Copy link
Member

nevkontakte commented May 11, 2022

@samhocevar this change looks good to me and I don't mind merging it, but I would like to better understand why it is needed in the first place. In a browser, I wouldn't expect it make it past the argv != js.Undefined. Usually such counter-intuitive behavior is a sign of another latent bug and I'd like to find it.

Do you know where argv gets set? Or could you attach an archive or a git repo which reproduces the crash so that I could poke at it?

@nevkontakte nevkontakte self-requested a review May 11, 2022 20:22
@nevkontakte nevkontakte merged commit d7a9da2 into gopherjs:master May 11, 2022
@samhocevar
Copy link
Contributor Author

Hi! I have investigated this issue more thoroughly, and I believe this was all caused by me calling the main GopherJS-generated function without arguments: my_code() instead of my_code(this)… I’m sorry for wasting your time with this!

@nevkontakte
Copy link
Member

No worries, glad you figured it out!

@JounQin
Copy link
Contributor

JounQin commented May 24, 2022

@nevkontakte Hi, when would this be released? See mvdan/sh#864

@nevkontakte
Copy link
Member

@JounQin our plan was to release it with GopherJS 1.18. It was merged after #1111, which is a big enough of a change that we didn't feel appropriate to release as a patch-level version. So far I haven't had much time to really dig into 1.18, but I have a vacation coming up, so that should change :)

If you are comfortable doing so, you should be able to install the unreleased GopherJS version from master: go get github.com/gopherjs/gopherjs@master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants