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

Fix regession: index out of range panic in reference link (#172, #173) #250

Merged
merged 2 commits into from
Apr 21, 2016
Merged

Fix regession: index out of range panic in reference link (#172, #173) #250

merged 2 commits into from
Apr 21, 2016

Conversation

tomkwok
Copy link
Contributor

@tomkwok tomkwok commented Apr 1, 2016

The previous fix got reverted/removed by new commits.

@rtfb
Copy link
Collaborator

rtfb commented Apr 2, 2016

What's up with the broken test?

@tomkwok
Copy link
Contributor Author

tomkwok commented Apr 2, 2016

Fixed.

@rtfb
Copy link
Collaborator

rtfb commented Apr 3, 2016

Thanks! Could you also please add a test that reproduces the issue? That would greatly diminish the chances such a regression would reoccur.

@tomkwok
Copy link
Contributor Author

tomkwok commented Apr 3, 2016

Done. And this is the naughty string: []:<

miekg added a commit to miekg/mmark that referenced this pull request Apr 14, 2016
@dmitshur
Copy link
Collaborator

dmitshur commented Apr 15, 2016

I can confirm the test case causes a panic without the fix:

panic: runtime error: index out of range [recovered]
    panic: runtime error: index out of range

goroutine 52 [running]:
panic(0x17afe0, 0xc82000c070)
    /usr/local/go/src/runtime/panic.go:464 +0x3e6
testing.tRunner.func1(0xc8201a4090)
    /usr/local/go/src/testing/testing.go:467 +0x192
panic(0x17afe0, 0xc82000c070)
    /usr/local/go/src/runtime/panic.go:426 +0x4e9
github.com/russross/blackfriday.scanLinkRef(0xc820106400, 0xc820272f20, 0x4, 0x8, 0x3, 0x4, 0x4, 0x0, 0x0, 0x0)

And the fix resolves it, while all tests pass.

LGTM. Shall we merge this @rtfb?

@dmitshur
Copy link
Collaborator

Friendly ping @rtfb.

@rtfb
Copy link
Collaborator

rtfb commented Apr 21, 2016

Yes! Sorry for the delay. I wanted to drill down the history to see when the regression happened, but couldn't find time for it until last night... Turns out I broke it with bcd6dd8 :-/. Now when I have it figured out, I can merge it and investigate later.

Thanks @tomkwok and @shurcooL!

@rtfb rtfb merged commit 151efb0 into russross:master Apr 21, 2016
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.

3 participants