Fixes crash rendering broken gopher maps #128

sloum · 2020-01-31T03:47:16Z

sloum commented

2020-01-31 03:47:16 +00:00

The map in question is for: gopher://dataforge.network

There are some funky things going on with the gophermap. This updates the way bad lines are handled. It will now make empty lines still work and improves handling of improperly formed lines (or lines of just text with only one field). Comparing the new output to lynx there is output parity, which makes me feel good about the change.

The map in question is for: `gopher://dataforge.network` There are some funky things going on with the gophermap. This updates the way bad lines are handled. It will now make empty lines still work and improves handling of improperly formed lines (or lines of just text with only one field). Comparing the new output to lynx there is output parity, which makes me feel good about the change.

sloum self-assigned this 2020-01-31 03:47:16 +00:00

sloum added the

labels 2020-01-31 03:47:16 +00:00

sloum closed this pull request

2020-01-31 03:47:29 +00:00

asdf commented

2020-01-31 22:16:00 +00:00

Just curious, where did the crash actually occur? Interesting this crash does not show an error message.

sloum commented

2020-02-01 00:22:13 +00:00

I agree it is weird that no error or panic was visible. As best as I was able to tell it had to do with line 138 (in the current master branch) of gopher/gopher.go. I really do need to set up logging for this project so I can get better error output in situations like this.

If you comment lines 138-144 and build and run with the above listed address it rendered fine. So the issue is definitely in those lines. The adjustment I made worked... but I am still unclear on the exact issue. Their gopher map is messed up in a few places (which Bombadillo should be able to handle).

The areas I focused on:

string(line[0][0]) == "i"

This check seems unstable. If line[0] is "" then this would be a bad reference. So I updated to:

strings.HasPrefix(line[0], "i")

Which is a safe option for testing that. However, that alone did not seem to fix the issue. But either way it was a change that should exist so I left it.

I simplified the logic of the surrounding if/else statements a bit and that seemed to solve the problem. There is no logical reason why it should have done so. However, this does seem to provide stable and reliable results. Handling broken pages pretty well.

If you feel like diving into this at all, this has not been merged into master so you can visit (gopher://dataforge.network)[gopher://dataforge.network] with master and with develop and check out the differences. If you can find anything I missed I would definitely love to solve this deeper than the current "it works now" solution.

I will also open an issue to set up logging and maybe a system new boolean flag to toggle logging on or off?

I agree it is weird that no error or panic was visible. As best as I was able to tell it had to do with line 138 (in the current master branch) of `gopher/gopher.go`. I really do need to set up logging for this project so I can get better error output in situations like this. If you comment lines 138-144 and build and run with the above listed address it rendered fine. So the issue is definitely in those lines. The adjustment I made worked... but I am still unclear on the exact issue. Their gopher map is messed up in a few places (which Bombadillo should be able to handle). The areas I focused on: ```Go string(line[0][0]) == "i" ``` This check seems unstable. If `line[0]` is `""` then this would be a bad reference. So I updated to: ```Go strings.HasPrefix(line[0], "i") ``` Which is a safe option for testing that. However, that alone did not seem to fix the issue. But either way it was a change that should exist so I left it. I simplified the logic of the surrounding if/else statements a bit and that seemed to solve the problem. There is no logical reason why it should have done so. However, this does seem to provide stable and reliable results. Handling broken pages pretty well. If you feel like diving into this at all, this has _not_ been merged into `master` so you can visit (gopher://dataforge.network)[gopher://dataforge.network] with `master` and with `develop` and check out the differences. If you can find anything I missed I would definitely love to solve this deeper than the current "it works now" solution. I will also open an issue to set up logging and maybe a system new boolean flag to toggle logging on or off?

asdf commented

2020-02-01 04:05:04 +00:00

Great, thanks for the information on this.

I'm happy to take a look. So far, I've created a basic unit test to trigger the issue, and it looks like it is definitely parseMap() causing a panic on line 141 (it doesn't look like buildLink() is involved though), with a message like this:

API server listening at: 127.0.0.1:42849
panic: runtime error: index out of range [0] with length 0 [recovered]
	panic: runtime error: index out of range [0] with length 0

goroutine 22 [running]:
testing.tRunner.func1(0xc0000d2200)
	/usr/lib/go-1.13/src/testing/testing.go:874 +0x60f
panic(0x59cea0, 0xc0000ca0a0)
	/usr/lib/go-1.13/src/runtime/panic.go:679 +0x1e0
tildegit.org/sloum/bombadillo/gopher.parseMap(0x5b7af3, 0x1e, 0x0, 0x0, 0x0, 0x0, 0x0)
	/home/ffffff/projects/bombadillo/gopher/gopher.go:141 +0xaf7
tildegit.org/sloum/bombadillo/gopher.Test_parseMap.func1(0xc0000d2200)
	/home/ffffff/projects/bombadillo/gopher/gopher_test.go:35 +0x8e
testing.tRunner(0xc0000d2200, 0xc000088700)
	/usr/lib/go-1.13/src/testing/testing.go:909 +0x13c
created by testing.(*T).Run
	/usr/lib/go-1.13/src/testing/testing.go:960 +0x64f

The key parts are:

panic: runtime error: index out of range [0] with length 0 [recovered]
	panic: runtime error: index out of range [0] with length 0

and

tildegit.org/sloum/bombadillo/gopher.parseMap(0x5b7af3, 0x1e, 0x0, 0x0, 0x0, 0x0, 0x0)
	/home/ffffff/projects/bombadillo/gopher/gopher.go:141 +0xaf7

This is done using vscodium, which has gotests to generate the shell of the unit test (you just fill in a struct), plus a "debug test" button which runs the debugger on that single unit test (which works pretty well).

That might give you a clear idea of what is happening but I'll investigate this further to understand exactly what is happening.

Great, thanks for the information on this. I'm happy to take a look. So far, I've created a basic unit test to trigger the issue, and it looks like it is definitely `parseMap()` causing a panic on line 141 (it doesn't look like `buildLink()` is involved though), with a message like this: ```shell API server listening at: 127.0.0.1:42849 panic: runtime error: index out of range [0] with length 0 [recovered] panic: runtime error: index out of range [0] with length 0 goroutine 22 [running]: testing.tRunner.func1(0xc0000d2200) /usr/lib/go-1.13/src/testing/testing.go:874 +0x60f panic(0x59cea0, 0xc0000ca0a0) /usr/lib/go-1.13/src/runtime/panic.go:679 +0x1e0 tildegit.org/sloum/bombadillo/gopher.parseMap(0x5b7af3, 0x1e, 0x0, 0x0, 0x0, 0x0, 0x0) /home/ffffff/projects/bombadillo/gopher/gopher.go:141 +0xaf7 tildegit.org/sloum/bombadillo/gopher.Test_parseMap.func1(0xc0000d2200) /home/ffffff/projects/bombadillo/gopher/gopher_test.go:35 +0x8e testing.tRunner(0xc0000d2200, 0xc000088700) /usr/lib/go-1.13/src/testing/testing.go:909 +0x13c created by testing.(*T).Run /usr/lib/go-1.13/src/testing/testing.go:960 +0x64f ``` The key parts are: ```shell panic: runtime error: index out of range [0] with length 0 [recovered] panic: runtime error: index out of range [0] with length 0 ``` and ```shell tildegit.org/sloum/bombadillo/gopher.parseMap(0x5b7af3, 0x1e, 0x0, 0x0, 0x0, 0x0, 0x0) /home/ffffff/projects/bombadillo/gopher/gopher.go:141 +0xaf7 ``` This is done using vscodium, which has [gotests](https://github.com/cweill/gotests) to generate the shell of the unit test (you just fill in a struct), plus a "debug test" button which runs the debugger on that single unit test (which works pretty well). That might give you a clear idea of what is happening but I'll investigate this further to understand exactly what is happening.

sloum commented

2020-02-01 17:59:14 +00:00

Yeah, that does confirm it is what I was thinking: a bad array access. I am definitely feeling like I should not have rushed that fix through. I see what the issue is and believe I have resolved it. I have a PR open: #130

Sign in to join this conversation.

No reviewers