search

felixc / rexiv2

Rust library for read/write access to media-file metadata (Exif, XMP, and IPTC)
GNU General Public License v3.0
79 stars 17 forks source link

Fix #27, segmentation fault when c_tags is null #28

Closed eigan closed 6 years ago

eigan commented 6 years ago

Since I applied your while patch, was free_array_of_pointers still called after the loop and caused another segfault. Just tell me if you would like me to change to if instead :)

eigan commented 6 years ago

Doesn't look like the other methods will ever return NULL though. See gexiv2_metadata_get_exif_tags: https://github.com/GNOME/gexiv2/blob/master/gexiv2/gexiv2-metadata-exif.cpp#L70

Do you want me to remove these checks then?

felixc commented 6 years ago

Thank you for putting together a patch!

After reviewing the gexiv2 documentation and source, I agree that it looks like the other methods won't ever return null, and it's not necessary to add a check for them too.

For get_tag_multiple_strings, it also sounds like we should switch to an if check before the loop, to avoid the segfault on free after the loop.

Thank you so much again for reporting the bug and contributing!

eigan commented 6 years ago

@felixc Alright. I reverted the initial commit, switched to an if and squashed :)

Please check the diff again.

eigan commented 6 years ago

Would it make more sense to return Err? I am not sure what one would usually do in this case in Rust @felixc

On Sun., 28 Jan. 2018, 23:15 Felix Crux, notifications@github.com wrote:

Merged #28 https://github.com/felixc/rexiv2/pull/28.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/felixc/rexiv2/pull/28#event-1445372617, or mute the thread https://github.com/notifications/unsubscribe-auth/AACjPRO_9DorDgMRq46-EVQ6or_nIJsLks5tPPFrgaJpZM4Rqzaz .

felixc commented 6 years ago

I'm not sure what the underlying exiv2 library's interpretation of a null return value is in this case... For example, is it an error in all cases, or does it sometimes just mean no results were found?

I don't think it's a Rust-specific question; we'd just need to investigate the intended meaning and return Err if that's what it's meant to mean.

Another option would be to check the type of the tag beforehand, and return a meaningful error type in the case where it isn't something that works with this function (i.e. not a collection).