Don't crash on invalid exiv2 data
ClosedPublic

Authored by poboiko on Oct 12 2018, 7:12 PM.

Details

Summary

The file from bug 375131 crashes baloo_file_extractor.
The problem is that its EXIF data contains a key Exif.Photo.FocalLength,
whose type is Exiv2::unsignedRational, and whose value is empty.
On the other hand, the Exiv2::Value::toFloat() call relies on at least single component of a value,
causing undefined behavior (i.e. crash) if there is none.

This is simple workaround: if we got a property with no value, just return an empty QVariant().
(unfortunately, didn't manage to reproduce the hang reported in the bug originally)

BUG: 352856
BUG: 353848
BUG: 361259
CCBUG: 375131

Test Plan

baloo_file_extractor no longer crashes on the file, it processes the file and extracts all the necessary data

Diff Detail

Repository
R286 KFileMetaData
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
poboiko created this revision.Oct 12 2018, 7:12 PM
Restricted Application added projects: Frameworks, Baloo. · View Herald TranscriptOct 12 2018, 7:12 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
poboiko requested review of this revision.Oct 12 2018, 7:12 PM

I guess bugs
352856
353848
361259
will also be fixed by this?

bruns added a subscriber: bruns.Oct 12 2018, 8:01 PM

Can you enhance the Summary a little bit?

It took me some time to notice Exiv2::Value::toFloat() is the same as Exiv2::Value::toFloat(0) (default argument), and accessing an inexisting element (i.e. n >= count()) triggers undefined behaviour, according to the Exiv2 documentation.

Can you remove the "tries to look for two numbers" from the summary - IMHO it is misleading, as although a rational consist of numerator and denumerator, it is still 1 element (i.e. count() >= 1 is sufficient).

Also, size() denotes the size in bytes, while we want the number of elements, i.e. count(), see Exiv2Extractor::fetchGpsDouble here and the Exiv2 API.

poboiko edited the summary of this revision. (Show Details)Oct 13 2018, 8:51 AM
poboiko updated this revision to Diff 43526.Oct 13 2018, 8:54 AM

Replaced size() by count(), which is more appropriate here

I guess bugs
352856
353848
361259
will also be fixed by this?

Yes, they should be. Thanks for the links! Added it to summary.

bruns added a comment.Oct 13 2018, 2:05 PM

I guess bugs
352856
353848
361259
will also be fixed by this?

Yes, they should be. Thanks for the links! Added it to summary.

These three should be CCBUG: (not test file, not able to confirm fixed), while 375131 should be BUG: (confirmed and fixed).

These three should be CCBUG: (not test file, not able to confirm fixed), while 375131 should be BUG: (confirmed and fixed).

Wait. Those three are about the very same crash I was able to reproduce (and fix) on the other test data. I'm pretty sure, looking at backtrace, they are due to the same reason.
(It's true that, without their test data I cannot be _completely_ sure, but looking at age of those bugs, I don't think users will be able to provide their files)

While the other one is about baloo hanging, which I wasn't able to reproduce, that's why it's just CC'd.

bruns added a comment.Oct 13 2018, 6:22 PM

These three should be CCBUG: (not test file, not able to confirm fixed), while 375131 should be BUG: (confirmed and fixed).

Wait. Those three are about the very same crash I was able to reproduce (and fix) on the other test data. I'm pretty sure, looking at backtrace, they are due to the same reason.
(It's true that, without their test data I cannot be _completely_ sure, but looking at age of those bugs, I don't think users will be able to provide their files)

While the other one is about baloo hanging, which I wasn't able to reproduce, that's why it's just CC'd.

Unfortunately 375131 is quite vague, they all speak about "baloo" hanging, but as "baloo" is not a single process, it is impossible to tell for sure which process crashes or hangs.

Apparent hangs of baloo_file are easy to produce, create a file 'baloo_file_extractor' with the following content:

#! /bin/bash
echo "S foobar"

make it executable, and then (after killing the current baloo_file), run PATH=./<path_with_fake_extractor> <path_to>/baloo_file. When you run baloo_monitor, you will see it says "Indexing foobar", but will make no further progress as the fake extractor has exited (equivalent to a crash here).

When you run baloo_file from build/bin, make sure you have the relevant baloo_file_extractor in the PATH first.

Unfortunately 375131 is quite vague, they all speak about "baloo" hanging, but as "baloo" is not a single process, it is impossible to tell for sure which process crashes or hangs.

Apparent hangs of baloo_file are easy to produce, create a file 'baloo_file_extractor' with the following content:

#! /bin/bash
echo "S foobar"

make it executable, and then (after killing the current baloo_file), run PATH=./<path_with_fake_extractor> <path_to>/baloo_file. When you run baloo_monitor, you will see it says "Indexing foobar", but will make no further progress as the fake extractor has exited (equivalent to a crash here).

When you run baloo_file from build/bin, make sure you have the relevant baloo_file_extractor in the PATH first.

Yeah, that's true. I guess that should be also not too hard to fix.
But there is two parts of the bug. The first one is what you mentioned, while the second is about baloo_file_extractor eating 100% CPU part, which is a bit more "interesting"...

bruns added a comment.Oct 14 2018, 9:24 PM

Yeah, that's true. I guess that should be also not too hard to fix.
But there is two parts of the bug. The first one is what you mentioned, while the second is about baloo_file_extractor eating 100% CPU part, which is a bit more "interesting"...

The 100% CPU is probably fixed with D12335.

For extractor crashes, I created T9867.

astippich accepted this revision.Oct 15 2018, 8:11 PM
This revision is now accepted and ready to land.Oct 15 2018, 8:11 PM
This revision was automatically updated to reflect the committed changes.