From b72efdb7aa25bd6e44d533e1a341a40a97c77fe0 Mon Sep 17 00:00:00 2001 From: Anton Sergeyev Date: Fri, 1 Apr 2022 11:09:11 +0600 Subject: [PATCH 1/5] =?UTF-8?q?=D0=BF=D1=80=D0=BE=D0=B1=D1=83=D0=B5=D0=BC?= =?UTF-8?q?=20=D0=BE=D1=82=D0=BB=D0=BE=D0=B2=D0=B8=D1=82=D1=8C=20=D0=BF?= =?UTF-8?q?=D1=80=D0=BE=D0=B1=D0=BB=D0=B5=D0=BC=D1=83=20=D1=81=20=D0=BE?= =?UTF-8?q?=D1=88=D0=B8=D0=B1=D0=BA=D0=BE=D0=B9=20`unexpected=20fault=20ad?= =?UTF-8?q?dress=200x7f45b5aea1f4`=20=D0=BD=D0=B0=20=D1=81=D1=82=D1=80?= =?UTF-8?q?=D0=BE=D0=BA=D0=B5=20`target=20:=3D=20make([]byte,=20size)`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- exiv.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/exiv.go b/exiv.go index 93e3047..5c72671 100644 --- a/exiv.go +++ b/exiv.go @@ -7,6 +7,7 @@ import "C" import ( "errors" + "fmt" "reflect" "runtime" "unsafe" @@ -114,13 +115,21 @@ func (i *Image) GetBytes() []byte { size := int(C.exiv_image_get_size(i.img)) ptr := C.exiv_image_get_bytes_ptr(i.img) + if size < 1 { + panic(fmt.Sprintf("invalid image size: %d", size)) + } + + if ptr < 1 { + panic(fmt.Sprintf("invalid image pointer: %v", ptr)) + } + var slice []byte header := (*reflect.SliceHeader)(unsafe.Pointer(&slice)) header.Cap = size header.Len = size header.Data = uintptr(unsafe.Pointer(ptr)) - target := make([]byte, len(slice)) + target := make([]byte, size) copy(target, slice) return target From 7c0760aed9b43c74669cd60582c61c570b7ac3fa Mon Sep 17 00:00:00 2001 From: Anton Sergeyev Date: Fri, 1 Apr 2022 11:18:11 +0600 Subject: [PATCH 2/5] =?UTF-8?q?=D0=BF=D1=80=D0=BE=D0=B1=D1=83=D0=B5=D0=BC?= =?UTF-8?q?=20=D0=BF=D0=BE=D1=87=D0=B8=D0=BD=D0=B8=D1=82=D1=8C=20=D1=81?= =?UTF-8?q?=D0=B1=D0=BE=D1=80=D0=BA=D1=83?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .travis.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 79ce9a7..c6cab50 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,5 +1,5 @@ language: go -go: 1.13 +go: 1.18 go_import_path: https://github.com/kolesa-team/goexiv cache: @@ -22,7 +22,7 @@ before_install: test -d exiv2-${EXIV2_VERSION} && { cd exiv2-${EXIV2_VERSION}/build } || { - wget http://www.exiv2.org/builds/exiv2-${EXIV2_VERSION}-Source.tar.gz + wget https://github.com/Exiv2/exiv2/releases/download/v${EXIV2_VERSION}/exiv2-${EXIV2_VERSION}-Source.tar.gz tar xzf exiv2-${EXIV2_VERSION}-Source.tar.gz mv exiv2-${EXIV2_VERSION}-Source exiv2-${EXIV2_VERSION} cd exiv2-${EXIV2_VERSION} From 23f2ecec9774016ac09283dc7195ef9bfbe2a5fc Mon Sep 17 00:00:00 2001 From: Anton Sergeyev Date: Fri, 1 Apr 2022 11:34:48 +0600 Subject: [PATCH 3/5] =?UTF-8?q?=D0=BF=D1=80=D0=BE=D0=B1=D1=83=D0=B5=D0=BC?= =?UTF-8?q?=20=D0=BF=D0=BE=D1=87=D0=B8=D0=BD=D0=B8=D1=82=D1=8C=20=D1=81?= =?UTF-8?q?=D0=B1=D0=BE=D1=80=D0=BA=D1=83?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- exiv.go | 4 +++- go.mod | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/exiv.go b/exiv.go index 5c72671..8a59cf8 100644 --- a/exiv.go +++ b/exiv.go @@ -119,7 +119,9 @@ func (i *Image) GetBytes() []byte { panic(fmt.Sprintf("invalid image size: %d", size)) } - if ptr < 1 { + dataPtr := uintptr(unsafe.Pointer(ptr)) + + if dataPtr < 1 { panic(fmt.Sprintf("invalid image pointer: %v", ptr)) } diff --git a/go.mod b/go.mod index 0338a81..ec2f055 100644 --- a/go.mod +++ b/go.mod @@ -6,4 +6,4 @@ require ( github.com/stretchr/testify v1.2.2 ) -go 1.13 +go 1.18 From bc911c418ae787d46078a6e4cacff13469b3d4b2 Mon Sep 17 00:00:00 2001 From: Anton Sergeyev Date: Mon, 4 Apr 2022 16:38:33 +0600 Subject: [PATCH 4/5] =?UTF-8?q?=D0=BF=D1=80=D0=BE=D0=B1=D1=83=D0=B5=D0=BC?= =?UTF-8?q?=20=D1=81=D1=82=D0=B0=D0=B1=D0=B8=D0=BB=D0=B8=D0=B7=D0=B8=D1=80?= =?UTF-8?q?=D0=BE=D0=B2=D0=B0=D1=82=D1=8C=20=D0=BF=D0=BE=D0=B2=D0=B5=D0=B4?= =?UTF-8?q?=D0=B5=D0=BD=D0=B8=D0=B5=20=D0=B2=20=D0=B3=D0=BE=D1=80=D1=83?= =?UTF-8?q?=D1=82=D0=B8=D0=BD=D0=B0=D1=85=20=D1=81=20=D0=BF=D0=BE=D0=BC?= =?UTF-8?q?=D0=BE=D1=89=D1=8C=D1=8E=20runtime.KeepAlive?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- README.md | 4 ++++ exiv.go | 54 ++++++++++++++++++++++------------------------------ exiv_test.go | 40 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 31 deletions(-) diff --git a/README.md b/README.md index 4614002..50ea6cf 100644 --- a/README.md +++ b/README.md @@ -71,6 +71,10 @@ if err != nil { fmt.Println(userComment) // "A comment. Might be a JSON string. Можно писать и по-русски!" + +// It is advisable to insert this line at the end of `goexivImg` lifecycle. +// see exiv_test.go:Test_GetBytes_Goroutine +runtime.KeepAlive(goexivImg) ``` Changing the image metadata in memory and returning the updated image (an approach fit for a web service): diff --git a/exiv.go b/exiv.go index 8a59cf8..7f6a6aa 100644 --- a/exiv.go +++ b/exiv.go @@ -7,8 +7,6 @@ import "C" import ( "errors" - "fmt" - "reflect" "runtime" "unsafe" ) @@ -19,7 +17,8 @@ type Error struct { } type Image struct { - img *C.Exiv2Image + bytesArrayPtr unsafe.Pointer + img *C.Exiv2Image } type MetadataProvider interface { @@ -43,13 +42,18 @@ func makeError(cerr *C.Exiv2Error) *Error { } } -func makeImage(cimg *C.Exiv2Image) *Image { +func makeImage(cimg *C.Exiv2Image, bytesPtr unsafe.Pointer) *Image { img := &Image{ - cimg, + bytesArrayPtr: bytesPtr, + img: cimg, } runtime.SetFinalizer(img, func(x *Image) { C.exiv2_image_free(x.img) + + if x.bytesArrayPtr != nil { + C.free(x.bytesArrayPtr) + } }) return img @@ -72,18 +76,25 @@ func Open(path string) (*Image, error) { return nil, err } - return makeImage(cimg), nil + return makeImage(cimg, nil), nil } // OpenBytes opens a byte slice with image data and returns a pointer to // the corresponding Image object, but does not read the Metadata. // Start the parsing with a call to ReadMetadata() -func OpenBytes(b []byte) (*Image, error) { - if len(b) == 0 { +func OpenBytes(input []byte) (*Image, error) { + if len(input) == 0 { return nil, &Error{0, "input is empty"} } + var cerr *C.Exiv2Error - cimg := C.exiv2_image_factory_open_bytes((*C.uchar)(unsafe.Pointer(&b[0])), C.long(len(b)), &cerr) + + bytesArrayPtr := C.CBytes(input) + cimg := C.exiv2_image_factory_open_bytes( + (*C.uchar)(bytesArrayPtr), + C.long(len(input)), + &cerr, + ) if cerr != nil { err := makeError(cerr) @@ -91,7 +102,7 @@ func OpenBytes(b []byte) (*Image, error) { return nil, err } - return makeImage(cimg), nil + return makeImage(cimg, bytesArrayPtr), nil } // ReadMetadata reads the metadata of an Image @@ -112,29 +123,10 @@ func (i *Image) ReadMetadata() error { // Returns an image contents. // If its metadata has been changed, the changes are reflected here. func (i *Image) GetBytes() []byte { - size := int(C.exiv_image_get_size(i.img)) + size := C.exiv_image_get_size(i.img) ptr := C.exiv_image_get_bytes_ptr(i.img) - if size < 1 { - panic(fmt.Sprintf("invalid image size: %d", size)) - } - - dataPtr := uintptr(unsafe.Pointer(ptr)) - - if dataPtr < 1 { - panic(fmt.Sprintf("invalid image pointer: %v", ptr)) - } - - var slice []byte - header := (*reflect.SliceHeader)(unsafe.Pointer(&slice)) - header.Cap = size - header.Len = size - header.Data = uintptr(unsafe.Pointer(ptr)) - - target := make([]byte, size) - copy(target, slice) - - return target + return C.GoBytes(unsafe.Pointer(ptr), C.int(size)) } // PixelWidth returns the width of the image in pixels diff --git a/exiv_test.go b/exiv_test.go index 0b20b4d..90f1007 100644 --- a/exiv_test.go +++ b/exiv_test.go @@ -5,6 +5,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "io/ioutil" + "runtime" + "sync" "testing" ) @@ -407,6 +409,44 @@ func Test_GetBytes(t *testing.T) { ) } +// Ensures image manipulation doesn't fail when running from multiple goroutines +func Test_GetBytes_Goroutine(t *testing.T) { + var wg sync.WaitGroup + iterations := 0 + + bytes, err := ioutil.ReadFile("testdata/stripped_pixel.jpg") + require.NoError(t, err) + + for i := 0; i < 100; i++ { + iterations++ + wg.Add(1) + + go func(i int) { + defer wg.Done() + + img, err := goexiv.OpenBytes(bytes) + require.NoError(t, err) + + // trigger garbage collection to increase the chance that underlying img.img will be collected + runtime.GC() + + bytesAfter := img.GetBytes() + assert.NotEmpty(t, bytesAfter) + + // if this line is removed, then the test will likely fail + // with segmentation violation. + // so far we couldn't come up with a better solution. + runtime.KeepAlive(img) + }(i) + } + + wg.Wait() + runtime.GC() + var memStats runtime.MemStats + runtime.ReadMemStats(&memStats) + t.Logf("Allocated bytes after test: %+v\n", memStats.HeapAlloc) +} + // Fills the image with metadata func initializeImage(path string, t *testing.T) { img, err := goexiv.Open(path) From 40377b127f620b1f5c6628e4286daf757fa8c4d7 Mon Sep 17 00:00:00 2001 From: Anton Sergeyev Date: Mon, 4 Apr 2022 16:52:49 +0600 Subject: [PATCH 5/5] =?UTF-8?q?=D0=B1=D0=B5=D0=BD=D1=87=D0=BC=D0=B0=D1=80?= =?UTF-8?q?=D0=BA=D0=B8?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- exiv_test.go | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/exiv_test.go b/exiv_test.go index 90f1007..c2e8ee8 100644 --- a/exiv_test.go +++ b/exiv_test.go @@ -447,6 +447,52 @@ func Test_GetBytes_Goroutine(t *testing.T) { t.Logf("Allocated bytes after test: %+v\n", memStats.HeapAlloc) } +func BenchmarkImage_GetBytes_KeepAlive(b *testing.B) { + bytes, err := ioutil.ReadFile("testdata/stripped_pixel.jpg") + require.NoError(b, err) + var wg sync.WaitGroup + + for i := 0; i < b.N; i++ { + wg.Add(1) + go func() { + defer wg.Done() + + img, err := goexiv.OpenBytes(bytes) + require.NoError(b, err) + + runtime.GC() + + require.NoError(b, img.SetExifString("Exif.Photo.UserComment", "123")) + + bytesAfter := img.GetBytes() + assert.NotEmpty(b, bytesAfter) + runtime.KeepAlive(img) + }() + } + + wg.Wait() +} + +func BenchmarkImage_GetBytes_NoKeepAlive(b *testing.B) { + bytes, err := ioutil.ReadFile("testdata/stripped_pixel.jpg") + require.NoError(b, err) + var wg sync.WaitGroup + + for i := 0; i < b.N; i++ { + wg.Add(1) + go func() { + defer wg.Done() + img, err := goexiv.OpenBytes(bytes) + require.NoError(b, err) + + require.NoError(b, img.SetExifString("Exif.Photo.UserComment", "123")) + + bytesAfter := img.GetBytes() + assert.NotEmpty(b, bytesAfter) + }() + } +} + // Fills the image with metadata func initializeImage(path string, t *testing.T) { img, err := goexiv.Open(path)