camlistored crashes due to unrecovered panic in goexif

[GoogleCodeExporter]

Steps to reproduce:

1) Fresh camlistored, e.g. 'devcam server -wipe -openbrowser'
2) Add IMAG0011.jpg (attached)

First lines of panic log: (full log attached):

panic: Tag format is not 'rational'

goroutine 503 [running]:
runtime.panic(0x9c5460, 0x1f7ea828)
    c:/go/src/pkg/runtime/panic.c:279 +0xe9
camlistore.org/third_party/github.com/camlistore/goexif/tiff.(*Tag).Rat2(0x1fb33
a90, 0x0, 0x1, 0x743a66, 0x9c0de0, 0x1f6d8e00)
    c:/Users/dsde012/go/src/camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/third_party/github.com/camlistore/goexif/tiff/tag.go:238 +0x6f
camlistore.org/third_party/github.com/camlistore/goexif/exif.tagDegrees(0x1fb33a
90, 0xb6c538, 0xe)
    c:/Users/dsde012/go/src/camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/third_party/github.com/camlistore/goexif/exif/exif.go:232 +0x3e
camlistore.org/third_party/github.com/camlistore/goexif/exif.(*Exif).LatLong(0x1
f7ea818, 0x0, 0x0, 0x0, 0x0, 0x0)
    c:/Users/dsde012/go/src/camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/third_party/github.com/camlistore/goexif/exif/exif.go:254 +0x147
camlistore.org/pkg/schema.FileTime(0x2b04a8, 0x1f6d8c60, 0x0, 0x0, 0x0, 0x0, 
0x0, 0x0)
    c:/Users/dsde012/go/src/camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/pkg/schema/schema.go:957 +0x291
camlistore.org/pkg/index.(*Index).populateFile(0x1f7c5e50, 0x2ac678, 
0x1f9ba8a0, 0x1f9ba860, 0x1fb244d0, 0x0, 0x0)
    c:/Users/dsde012/go/src/camlistore.org/tmp/build-gopath-nosqlite/src/camlistore.org/pkg/index/receive.go:399 +0x9c8

Nevermind it ran on Windows as it's also reproducable on my Linux box.

Original issue reported on code.google.com by dsimfab...@gmail.com on 25 Aug 2014 at 10:31

Attachments:

• IMAG0011.jpg
• paniclog.txt

[GoogleCodeExporter]

I reported an issue for goexif as well:
https://github.com/rwcarlsen/goexif/issues/23

Original comment by dsimfab...@gmail.com on 25 Aug 2014 at 10:56

[GoogleCodeExporter]

I pushed a fix: https://camlistore-review.googlesource.com/#/c/3637/

Original comment by dsimfab...@gmail.com on 28 Aug 2014 at 1:47

[GoogleCodeExporter]

Hmm, I see you've been communicating with rwcarlsen already. So why not propose 
that change to goexif directly, so we can then simply pull from upstream 
afterwards, and hence avoid diverging whenever possible?

Original comment by mathieu....@gmail.com on 28 Aug 2014 at 4:48

[GoogleCodeExporter]

I agree that another merger round with goexif upstream is due. I will be happy 
to take on that task with another set of patches at a later time. For two 
reasons:

a) rwcarlsen is currently changing the goexif API [1] (per an issue reported by 
me, so all blame on me if something gets broken). I'd rather merge these API 
changes into the camlistore thirdparty code back first before I send him any 
feature additions.

b) My changes only affect goexif code that camlistore added (the feature funcs 
tagDegrees, LatLong). I wanted to present them to the camlistore community to 
review first as currently these changes make sense in the context of the the 
camlistore version of goexif only. 

[1] https://github.com/rwcarlsen/goexif/pull/24

Original comment by dsimfab...@gmail.com on 28 Aug 2014 at 6:02

[GoogleCodeExporter]

I'm happy to do reviews on our Camlistore Gerrit first, but I really don't want 
to diverge and even have a "version" of things where possible.  Perhaps we 
should be a co-owner of the upstream, if the maintainer would permit that.

Original comment by bradfitz on 28 Aug 2014 at 6:04

[GoogleCodeExporter]

Sounds reasonable. However, it diverged already since 2013. Allow me to 
illustrate with an example:

The package goexif/tiff has a type that describes the various tag types. Its 
original name was FormatType, it was renamed upstream in January 2013 to 
TypeCategory [1]. The commit [1] was merged to https://github.com/camlistore in 
a Pull Request [2] at 8th October 2013. However, a Change [3] of some days 
later (which claimed to merge thirdparty/.../goexif to upstream and even 
introduces a procedure for that) does NOT contain this name change.

Bill Thiede wrote in the commit message of [3]:  "[...]we will need a followup 
commit that manually merges
changes that failed during 'git am'." Such a followup commit never came.

Instead we had three commits that never made it to upstream goexif. [4]

I cannot help but agree with the gist of the discussion in [3]: Merging all 
this is a pain. 

However, here's my plan: 
a) I wait for rwcarlsen to finish and merge the PR mentioned above [5] in 
upstream goexif, 
b) I start another round of merging upstream goexif to camlistore/third_party 
and 
c) I create a PR for upstream goexif with all stuff that you guys added locally 
and my fix for this issue.

Are you okay with that?

[1] 
https://github.com/rwcarlsen/goexif/commit/fc165b6c17b16fead7635643622536a87e339
913
[2] https://github.com/camlistore/goexif/pull/3
[3] https://camlistore-review.googlesource.com/#/c/1116/
[4] 
https://camlistore.googlesource.com/camlistore/+log/b6d3d1a1037/third_party/gith
ub.com/camlistore/goexif
[5] https://github.com/rwcarlsen/goexif/pull/24

Original comment by dsimfab...@gmail.com on 28 Aug 2014 at 8:33

[GoogleCodeExporter]

sgtm

Original comment by mathieu....@gmail.com on 28 Aug 2014 at 8:42

[GoogleCodeExporter]

59a451c2dc39a0309b04415683a3d8e8be7fe17b
2aed1b8241530049e3ded6a00043cc3f8faa746b
f0d9c04bc27677a6f72e6a634f380d26739a273b

Original comment by mathieu....@gmail.com on 19 Sep 2014 at 2:41

• Changed state: Fixed

[GoogleCodeExporter]

This issue has moved to https://camlistore.org/issue/498

Original comment by bradfitz on 14 Dec 2014 at 11:37

• Added labels: IssueMoved, Restrict-AddIssueComment-Commit