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.

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

x/image/font/sfnt: GlyphIndex panic after multiple calls without buffer #46948

Closed
mokiat opened this issue Jun 27, 2021 · 5 comments
Closed

x/image/font/sfnt: GlyphIndex panic after multiple calls without buffer #46948

mokiat opened this issue Jun 27, 2021 · 5 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@mokiat
Copy link

mokiat commented Jun 27, 2021

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

$ go version
go version go1.16.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
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/momchil/.cache/go-build"
GOENV="/home/momchil/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/momchil/.go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/momchil/.go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go-1.16"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go-1.16/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16.5"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/momchil/[redacted]/go.mod"
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-build80210502=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I am parsing a set of Roboto font files in a sequence. All of this occurs on a single thread/go routine (I ran it with -race just in case).

When parsing each font file I am checking the bounds of the glyph M via font.GlyphBounds. In order to do that, I need to get the index of the glyph via the font.GlyphIndex method. I am passing nil for buffer, which according to the documentation is allowed.

On the 9th font to be processed, the application crashes with a nil pointer dereference.

This does not reproduce itself if I pass a buffer object. Using a dedicated buffer per iteration or a shared buffer for all invocations works.

What did you expect to see?

What did you see instead?

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x60bd0d]

goroutine 1 [running]:
golang.org/x/image/font/sfnt.(*Buffer).view(0x0, 0xc000082c00, 0xc98ae, 0x2, 0xc0001d1320, 0x0, 0x0, 0x0, 0xc00010e050)
	/home/momchil/.go/pkg/mod/golang.org/x/image@v0.0.0-20210622092929-e6eecd499c2c/font/sfnt/sfnt.go:1902 +0x4d
golang.org/x/image/font/sfnt.(*Font).makeCachedGlyphIndexFormat4.func1(0xc000082c00, 0x0, 0xc00000004d, 0x3, 0x3, 0xcae000005dc)
	/home/momchil/.go/pkg/mod/golang.org/x/image@v0.0.0-20210622092929-e6eecd499c2c/font/sfnt/cmap.go:189 +0x30b
golang.org/x/image/font/sfnt.(*Font).GlyphIndex(...)
	/home/momchil/.go/pkg/mod/golang.org/x/image@v0.0.0-20210622092929-e6eecd499c2c/font/sfnt/sfnt.go:1378
...

The font files that I parse are as follows (where I have packed them in a Collection):

2021/06/27 19:44:33 running application
2021/06/27 19:44:36 font: roboto / regular
2021/06/27 19:44:36 ascent: 89:04, descent: 23:28, lineheight: 112:32
2021/06/27 19:44:36 char M ascent: -68:16, descent: 0:00, height: 68:16
2021/06/27 19:44:36 font: roboto thin / regular
2021/06/27 19:44:36 ascent: 89:04, descent: 23:28, lineheight: 112:32
2021/06/27 19:44:36 char M ascent: -68:16, descent: 0:00, height: 68:16
2021/06/27 19:44:36 font: roboto thin / italic
2021/06/27 19:44:36 ascent: 89:04, descent: 23:28, lineheight: 112:32
2021/06/27 19:44:36 char M ascent: -68:16, descent: 0:00, height: 68:16
2021/06/27 19:44:36 font: roboto medium / regular
2021/06/27 19:44:36 ascent: 89:04, descent: 23:28, lineheight: 112:32
2021/06/27 19:44:36 char M ascent: -68:16, descent: 0:00, height: 68:16
2021/06/27 19:44:36 font: roboto medium / italic
2021/06/27 19:44:36 ascent: 89:04, descent: 23:28, lineheight: 112:32
2021/06/27 19:44:36 char M ascent: -68:16, descent: 0:00, height: 68:16
2021/06/27 19:44:37 font: roboto light / regular
2021/06/27 19:44:37 ascent: 89:04, descent: 23:28, lineheight: 112:32
2021/06/27 19:44:37 char M ascent: -68:16, descent: 0:00, height: 68:16
2021/06/27 19:44:37 font: roboto light / italic
2021/06/27 19:44:37 ascent: 89:04, descent: 23:28, lineheight: 112:32
2021/06/27 19:44:37 char M ascent: -68:16, descent: 0:00, height: 68:16
2021/06/27 19:44:37 font: roboto / italic
2021/06/27 19:44:37 ascent: 89:04, descent: 23:28, lineheight: 112:32
2021/06/27 19:44:37 char M ascent: -68:16, descent: 0:00, height: 68:16
2021/06/27 19:44:37 font: roboto / bold
2021/06/27 19:44:37 ascent: 89:04, descent: 23:28, lineheight: 112:32
[CRASH HERE]
@gopherbot gopherbot added this to the Unreleased milestone Jun 27, 2021
@smasher164 smasher164 added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 27, 2021
@smasher164
Copy link
Member

/cc @nigeltao

@nigeltao
Copy link
Contributor

Can you attach the program (and ideally the .ttc file) too?

@mokiat
Copy link
Author

mokiat commented Jun 28, 2021

I wrote a more minimalistic program that reproduces the problem: sfntpanic.zip
Hope this helps.

@tinne26
Copy link

tinne26 commented Mar 22, 2022

I encountered this in the wild too and looked into it: when GlyphIndex is called, it passes the job to a glyphIndexFunc. Since there are different types of glyphIndexFuncs, some work and others don't. In particular, the function created in makeCachedGlyphIndexFormat4 doesn't take into account the case of a nil buffer. The error clearly indicates the crash happens at line 189, where it can be seen that the buffer is being used without having tested whether it's nil or not first. The fix should be as simple as inserting the following around line 173 (the same that is done in many other parts of the code):

if b == nil {
	b = &Buffer{}
}

None of the other formats seem to be affected, as only format 4 is actually using the buffer. Might be a good idea to change the signatures of the other formats to _ *Buffer to make it clearer.

@gopherbot
Copy link

Change https://go.dev/cl/398774 mentions this issue: font/sfnt: fix nil Buffer dereference

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 7, 2022
tinne26 added a commit to tinne26/etxt that referenced this issue Jul 24, 2022
@tinne26
Bumping sfnt and text versions.
e3166d0 
Mostly for the panic fix on sfnt for 
golang/go#46948.
@golang golang locked and limited conversation to collaborators Apr 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@dmitshur @mokiat @nigeltao @gopherbot @smasher164 @tinne26