adamdruppe / arsd

This is a collection of modules that I've released over the years. Most of them stand alone, or have just one or two dependencies in here, so you don't have to download this whole repo.
http://arsd-official.dpldocs.info/arsd.html
531 stars 128 forks source link

Segfault in JPEG loader #331

Closed arpie42 closed 2 years ago

arpie42 commented 2 years ago

I know next to nothing about JPEG decoding so am probably missing something obvious. The attached JPEG (and apparently most, but not all, photos taken by my Samsung Galaxy Note) crashes the arsd.jpeg loader with a segmentation fault. I'm basically doing :

auto image = readJpeg(fileName);

followed by any read function (image.getPixel, image.width, image.getAsTrueColorImage()...).

The segfault is happening on calling getPixel() or similar, but I am assuming that is just a red herring and that there is some lazy loading going on in the background.

I'm guessing that this isn't actually a bug, more likely my phone is saving images in a format that arsd.jpeg doesn't support... would it be possible to easily add support? I'm happy to have a hack, but wanted to check first that I wouldn't be wasting my time? If you could point me in the direction of any jpeg file format specifications, that would probably be helpful!

Or, then again, maybe it is a bug...?

Crash !

adamdruppe commented 2 years ago

Possible it is a bug, or even just metainfo it didn't skip like it should.

Wife out and i have the baby right now, I'll see if I can test it in about an hour, but no promises I'll have a fix until at least Friday, since work gonna be busy next couple days too. Let's hope it is an easy fix and won't take that long!

but i'll get back to you in a bit here.

adamdruppe commented 2 years ago

error code JPGD_UNEXPECTED_MARKER as it reads deep into the file, looking for an end of image marker, causing it to return null. now the question is why but again ill have to come back to it.

adamdruppe commented 2 years ago

there's a reset thing right before the end. that should be harmless and probably simply ignored instead of returning an error. gotta check the spec though to make sure im not missing anything.

adamdruppe commented 2 years ago

I think this fixes it: https://github.com/adamdruppe/arsd/commit/f38b1ff4011d259eb6e8c8c5fd5abf1fdb6039be

seems to work for me on the test image but I didn't regression test or anything either.

could you retest on your end with the new master jpeg.d with a variety of files and let me know?

thanks for reporting btw!

arpie42 commented 2 years ago

Wow, that was fast! It seems fixed to me, certainly the few test files I have that were crashing before are now working fine. Thanks a lot!

But... more generally, is there anything that can be done to prevent segfaults if a similar but slightly different bug turns up with another file? I have tens of thousands of files that I want to process and it would be great to be able to automatically skip files that cause errors.

Before reporting, I tried to get something useful from gdb but it kept segfaulting at a call to getAsTrueColorImage and I couldn't find a way to drill down any further. Can you shed any light onto why? Or how to get a useful error message instead of a segfault? I probably won't have time to get back on it until the weekend so no rush!

Also, do you have a link to the jpeg spec?

adamdruppe commented 2 years ago

But... more generally, is there anything that can be done to prevent segfaults if a similar but slightly different bug turns up with another file?

Yeah, I should have said this in the doc, but readJpeg returns null if it can't read the file for whatever reason. So you can simply do:

auto img = readJpeg("filename.jpg");
if(img is null) {
     // load failed
}

// otherwise img is not null and you can use it

Your segfault was just because you tried to use a null object. (just since it was a jpg file my big concern is why it was null instead of working as it should have!)

Can you shed any light onto why?

Just because there was nothing further to drill down to; the object itself was null at the top level, so calling any method on it would fail.

Also, do you have a link to the jpeg spec?

I pulled one off the wikipedia footnotes lol

arpie42 commented 2 years ago

Dammit, now I feel stupid. I've been coding in Kotlin for too long! Of course it's a null pointer. Thanks for explaining it in such a non-patronising way :) I think we can consider this closed, I'll reopen if I find any other troublesome files. Thanks again.

adamdruppe commented 2 years ago

ok, thanks again for the report!