shiny: catch invalid modifications to a Buffer's RGBA.

A Buffer is essentially an *image.RGBA, but not all *image.RGBAs are
valid Buffers. This change detects invalid attempts to pass arbitrary
*image.RGBAs as Buffers. Prior to this change, this would 'work' for the
gldriver but not other drivers.

Fixes golang/go#14191.

Change-Id: I8b0d7f0861dc69b93d332bd0ba26a70326e16c7b
Reviewed-on: https://go-review.googlesource.com/19211
Reviewed-by: David Crawshaw <crawshaw@golang.org>

shiny/driver/gldriver/buffer.go

@@ -7,11 +7,26 @@ package gldriver
 import "image"
 
 type bufferImpl struct {
-	rgba *image.RGBA
+	// buf should always be equal to (i.e. the same ptr, len, cap as) rgba.Pix.
+	// It is a separate, redundant field in order to detect modifications to
+	// the rgba field that are invalid as per the screen.Buffer documentation.
+	buf  []byte
+	rgba image.RGBA
 	size image.Point
 }
 
 func (b *bufferImpl) Release()                {}
 func (b *bufferImpl) Size() image.Point       { return b.size }
 func (b *bufferImpl) Bounds() image.Rectangle { return image.Rectangle{Max: b.size} }
-func (b *bufferImpl) RGBA() *image.RGBA       { return b.rgba }
+func (b *bufferImpl) RGBA() *image.RGBA       { return &b.rgba }
+
+func (b *bufferImpl) preUpload() {
+	// Check that the program hasn't tried to modify the rgba field via the
+	// pointer returned by the bufferImpl.RGBA method. This check doesn't catch
+	// 100% of all cases; it simply tries to detect some invalid uses of a
+	// screen.Buffer such as:
+	//	*buffer.RGBA() = anotherImageRGBA
+	if len(b.buf) != 0 && len(b.rgba.Pix) != 0 && &b.buf[0] != &b.rgba.Pix[0] {
+		panic("gldriver: invalid Buffer.RGBA modification")
+	}
+}

shiny/driver/gldriver/screen.go

@@ -40,8 +40,10 @@ type screenImpl struct {
 }
 
 func (s *screenImpl) NewBuffer(size image.Point) (retBuf screen.Buffer, retErr error) {
+	m := image.NewRGBA(image.Rectangle{Max: size})
 	return &bufferImpl{
-		rgba: image.NewRGBA(image.Rectangle{Max: size}),
+		buf:  m.Pix,
+		rgba: *m,
 		size: size,
 	}, nil
 }

shiny/driver/gldriver/texture.go

@@ -32,12 +32,15 @@ func (t *textureImpl) Release() {
 }
 
 func (t *textureImpl) Upload(dp image.Point, src screen.Buffer, sr image.Rectangle) {
+	buf := src.(*bufferImpl)
+	buf.preUpload()
+
 	t.w.glctxMu.Lock()
 	defer t.w.glctxMu.Unlock()
 
 	// TODO: adjust if dp is outside dst bounds, or r is outside src bounds.
 	t.w.glctx.BindTexture(gl.TEXTURE_2D, t.id)
-	m := src.(*bufferImpl).rgba.SubImage(sr).(*image.RGBA)
+	m := buf.rgba.SubImage(sr).(*image.RGBA)
 	b := m.Bounds()
 	// TODO check m bounds smaller than t.size
 	t.w.glctx.TexSubImage2D(gl.TEXTURE_2D, 0, 0, 0, b.Dx(), b.Dy(), gl.RGBA, gl.UNSIGNED_BYTE, m.Pix)

shiny/driver/windriver/buffer.go

@@ -32,6 +32,15 @@ func (b *bufferImpl) Bounds() image.Rectangle { return image.Rectangle{Max: b.si
 func (b *bufferImpl) RGBA() *image.RGBA       { return &b.rgba }
 
 func (b *bufferImpl) preUpload(reusable bool) {
+	// Check that the program hasn't tried to modify the rgba field via the
+	// pointer returned by the bufferImpl.RGBA method. This check doesn't catch
+	// 100% of all cases; it simply tries to detect some invalid uses of a
+	// screen.Buffer such as:
+	//	*buffer.RGBA() = anotherImageRGBA
+	if len(b.buf) != 0 && len(b.rgba.Pix) != 0 && &b.buf[0] != &b.rgba.Pix[0] {
+		panic("windriver: invalid Buffer.RGBA modification")
+	}
+
 	b.mu.Lock()
 	defer b.mu.Unlock()
 

shiny/driver/x11driver/buffer.go

@@ -41,6 +41,15 @@ func (b *bufferImpl) Bounds() image.Rectangle { return image.Rectangle{Max: b.si
 func (b *bufferImpl) RGBA() *image.RGBA       { return &b.rgba }
 
 func (b *bufferImpl) preUpload() {
+	// Check that the program hasn't tried to modify the rgba field via the
+	// pointer returned by the bufferImpl.RGBA method. This check doesn't catch
+	// 100% of all cases; it simply tries to detect some invalid uses of a
+	// screen.Buffer such as:
+	//	*buffer.RGBA() = anotherImageRGBA
+	if len(b.buf) != 0 && len(b.rgba.Pix) != 0 && &b.buf[0] != &b.rgba.Pix[0] {
+		panic("x11driver: invalid Buffer.RGBA modification")
+	}
+
 	b.mu.Lock()
 	defer b.mu.Unlock()
 

shiny/screen/screen.go

@@ -80,7 +80,10 @@ type Screen interface {
 
 // Buffer is an in-memory pixel buffer. Its pixels can be modified by any Go
 // code that takes an *image.RGBA, such as the standard library's image/draw
-// package.
+// package. A Buffer is essentially an *image.RGBA, but not all *image.RGBA
+// values (including those returned by image.NewRGBA) are valid Buffers, as a
+// driver may assume that the memory backing a Buffer's pixels are specially
+// allocated.
 //
 // To see a Buffer's contents on a screen, upload it to a Texture (and then
 // draw the Texture on a Window) or upload it directly to a Window.
@@ -105,6 +108,31 @@ type Buffer interface {
 	// RGBA returns the pixel buffer as an *image.RGBA.
 	//
 	// Its contents should not be accessed while the Buffer is uploading.
+	//
+	// The contents of the returned *image.RGBA's Pix field (of type []byte)
+	// can be modified at other times, but that Pix slice itself (i.e. its
+	// underlying pointer, length and capacity) should not be modified at any
+	// time.
+	//
+	// The following is valid:
+	//	m := buffer.RGBA()
+	//	if len(m.Pix) >= 4 {
+	//		m.Pix[0] = 0xff
+	//		m.Pix[1] = 0x00
+	//		m.Pix[2] = 0x00
+	//		m.Pix[3] = 0xff
+	//	}
+	// or, equivalently:
+	//	m := buffer.RGBA()
+	//	m.SetRGBA(m.Rect.Min.X, m.Rect.Min.Y, color.RGBA{0xff, 0x00, 0x00, 0xff})
+	// and using the standard library's image/draw package is also valid:
+	//	dst := buffer.RGBA()
+	//	draw.Draw(dst, dst.Bounds(), etc)
+	// but the following is invalid:
+	//	m := buffer.RGBA()
+	//	m.Pix = anotherByteSlice
+	// and so is this:
+	//	*buffer.RGBA() = anotherImageRGBA
 	RGBA() *image.RGBA
 }