gomarkdown / markdown

markdown parser and HTML renderer for Go
Other
1.36k stars 171 forks source link

Parser panics when called `Parse` is called multiple times #280

Closed weberc2-tempus closed 1 year ago

weberc2-tempus commented 1 year ago

output

$ go run .
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x10c61f0]

goroutine 1 [running]:
github.com/gomarkdown/markdown/parser.canNodeContain({0x0?, 0x0}, {0x11163a8?, 0xc0000b81e0})
        /Users/craig.weber/.gvm/pkgsets/go1.19.2/global/pkg/mod/github.com/gomarkdown/markdown@v0.0.0-20230322041520-c84983bdbf2a/parser/parser.go:252 +0x2f0
github.com/gomarkdown/markdown/parser.(*Parser).addChild(0xc000104000, {0x11163a8?, 0xc0000b81e0})
        /Users/craig.weber/.gvm/pkgsets/go1.19.2/global/pkg/mod/github.com/gomarkdown/markdown@v0.0.0-20230322041520-c84983bdbf2a/parser/parser.go:214 +0x5a
github.com/gomarkdown/markdown/parser.(*Parser).AddBlock(0xc000104000, {0x11163a8, 0xc0000b81e0})
        /Users/craig.weber/.gvm/pkgsets/go1.19.2/global/pkg/mod/github.com/gomarkdown/markdown@v0.0.0-20230322041520-c84983bdbf2a/parser/block.go:392 +0x105
github.com/gomarkdown/markdown/parser.(*Parser).renderParagraph(0xc0000bc161?, {0xc0000bc161, 0x1?, 0x1})
        /Users/craig.weber/.gvm/pkgsets/go1.19.2/global/pkg/mod/github.com/gomarkdown/markdown@v0.0.0-20230322041520-c84983bdbf2a/parser/block.go:1593 +0x10a
github.com/gomarkdown/markdown/parser.(*Parser).paragraph(0xc000104000, {0xc0000bc161, 0x1, 0x1})
        /Users/craig.weber/.gvm/pkgsets/go1.19.2/global/pkg/mod/github.com/gomarkdown/markdown@v0.0.0-20230322041520-c84983bdbf2a/parser/block.go:1762 +0x597
github.com/gomarkdown/markdown/parser.(*Parser).Block(0xc000104000, {0xc0000bc161?, 0x11159a8?, 0xc00009e780?})
        /Users/craig.weber/.gvm/pkgsets/go1.19.2/global/pkg/mod/github.com/gomarkdown/markdown@v0.0.0-20230322041520-c84983bdbf2a/parser/block.go:373 +0xf3c
github.com/gomarkdown/markdown/parser.(*Parser).Parse(0xc000104000, {0xc0000bc161?, 0x0?, 0x10d1da0?})
        /Users/craig.weber/.gvm/pkgsets/go1.19.2/global/pkg/mod/github.com/gomarkdown/markdown@v0.0.0-20230322041520-c84983bdbf2a/parser/parser.go:292 +0xb9
main.main()
        /tmp/main.go:10 +0x6b
exit status 2

main.go

package main

import (
    "github.com/gomarkdown/markdown/parser"
)

func main() {
    p := parser.New()
    _ = p.Parse([]byte("i"))
    _ = p.Parse([]byte("i"))
}

go.mod

module tmp

go 1.19

require github.com/gomarkdown/markdown v0.0.0-20230322041520-c84983bdbf2a // indirect
kjk commented 1 year ago

consider that to be by design and don't do it

weberc2-tempus commented 1 year ago

Respectfully, this seems like a pretty error-prone design. If Parser objects are only intended to be used once, perhaps they could be made private with the public API being a package-level Parse() function that internally creates the parser object so it will never be reused like this? Something like this:

type parser struct { ... }

func newWithExtensions(extensions Extensions) *parser { return &parser{ ... } }

func Parse(data []byte, extensions Extensions) ast.Node {
    return newWithExtensions(extensions).parse(data)
}

If you're really opposed to that for whatever reason, could the documentation at least be updated to reflect this pitfall?

peterstace commented 1 year ago

I agree that under the current design, it's easy to run into this problem (I ran into the exact same problem as the OP just now). Documentation against the Parser type indicating that it's single-use would have helped.

I'll raise a PR to improve the documentation.