Skip to content

x/net/html: render function segfaults instead of returning an error #30125

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.

Sign up for GitHub

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

Jump to bottom
Closed
dj95 opened this issue Feb 7, 2019 · 8 comments
Closed

x/net/html: render function segfaults instead of returning an error #30125

dj95 opened this issue Feb 7, 2019 · 8 comments
Labels
FrozenDueToAge
Milestone
Unreleased

Comments

@dj95
Copy link

dj95 commented Feb 7, 2019

What version of Go are you using (go version)?

$ go version
go version go1.11.5 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/daniel/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/daniel/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/lib/go"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build547983775=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I put invalid nodes into the render function in order to test if my error handling is correct.
E.g. with the following code:

package main

import (
	"bytes"
	"fmt"
	"io"

	"golang.org/x/net/html"
)

func main() {
	// create a byte buffer
	var buf bytes.Buffer

	// create a writer for the buffer
	w := io.Writer(&buf)

	err := html.Render(w, nil)

	fmt.Printf("%s\n", err)
}

a segfault occurs instead of a returned error.

What did you expect to see?

I expect to see an error returned by the render function

What did you see instead?

The program crashes with a segmentation fault.
@gopherbot gopherbot added this to the Unreleased milestone Feb 7, 2019
@dj95
Copy link
Author

dj95 commented Feb 7, 2019

The same behaviour occurs, if I try to render a malformed tree of nodes.

@gjolly
Copy link

gjolly commented Feb 7, 2019

I think this could be easily fixed by checking if the pointer is nil before any processing in render1 (render.go:69).

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/161637 mentions this issue: x/net/html: fix a SegFault when trying to render an invalid node tree

@antong
Copy link
Contributor

antong commented Feb 7, 2019

I don't see an issue here. A panic when you pass nil to a function expecting a pointer is business as usual. The standard library also does this all over the place. If a nil pointer can be passed (e.g., a *Config with nil meaning a default config), then that should be clearly documented.

@mikioh
Copy link
Contributor

mikioh commented Feb 8, 2019 •
edited
Loading

The standard library also does this all over the place.

Well, yes and no, depending on the assessment; not simple nillable vs. non-nillable, perhaps from the system fault tolerance point of view.

@gjolly
Copy link

gjolly commented Feb 10, 2019

The same behaviour occurs, if I try to render a malformed tree of nodes.

@dj95 Could you give an example of a such malformed tree please?

@antong
Copy link
Contributor

antong commented Feb 10, 2019

@dj95, a minimal code example is the perfect bug report! However, the example is perhaps too contrived. How did you end up with a nil node in the first place?

@bcmills
Copy link
Contributor

bcmills commented Feb 28, 2019

As @antong notes, Go packages conventionally do not check explicitly for nil arguments. (If you want to avoid nil-panics, I recommend fuzz-testing your program.)

@bcmills bcmills closed this as completed Feb 28, 2019
@golang golang locked and limited conversation to collaborators Feb 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@mikioh @antong @dj95 @bcmills @gopherbot @gjolly