D16165: Don't crash on invalid exiv2 data

[Igor Poboiko]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:5eee9ac75b7d: Dont crash on invalid exiv2 data 
(authored by poboiko).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16165?vs=43526=43736

REVISION DETAIL
  https://phabricator.kde.org/D16165

AFFECTED FILES
  src/extractors/exiv2extractor.cpp

To: poboiko, #baloo, #frameworks, astippich
Cc: bruns, astippich, kde-frameworks-devel, ashaposhnikov, michaelh, spoorun, 
ngraham, abrahams

D16165: Don't crash on invalid exiv2 data

[Alexander Stippich]

astippich accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R286 KFileMetaData

BRANCH
  dont-crash-invalid-exiv (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D16165

To: poboiko, #baloo, #frameworks, astippich
Cc: bruns, astippich, kde-frameworks-devel, ashaposhnikov, michaelh, spoorun, 
ngraham, abrahams

D16165: Don't crash on invalid exiv2 data

[Stefan Brüns]

bruns added a comment.


  In D16165#342975 , @poboiko wrote:
  
  >
  
  
  
  
  > 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 .

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D16165

To: poboiko, #baloo, #frameworks
Cc: bruns, astippich, kde-frameworks-devel, ashaposhnikov, michaelh, spoorun, 
ngraham, abrahams

D16165: Don't crash on invalid exiv2 data

[Igor Poboiko]

poboiko added a comment.


  In D16165#342550 , @bruns wrote:
  
  > 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=./ /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"...

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D16165

To: poboiko, #baloo, #frameworks
Cc: bruns, astippich, kde-frameworks-devel, ashaposhnikov, michaelh, spoorun, 
ngraham, abrahams

D16165: Don't crash on invalid exiv2 data

[Stefan Brüns]

bruns added a comment.


  In D16165#342522 , @poboiko wrote:
  
  > In D16165#342433 , @bruns wrote:
  >
  > > 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=./ /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.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D16165

To: poboiko, #baloo, #frameworks
Cc: bruns, astippich, kde-frameworks-devel, ashaposhnikov, michaelh, spoorun, 
ngraham, abrahams

D16165: Don't crash on invalid exiv2 data

[Igor Poboiko]

poboiko added a comment.


  In D16165#342433 , @bruns wrote:
  
  > 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.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D16165

To: poboiko, #baloo, #frameworks
Cc: bruns, astippich, kde-frameworks-devel, ashaposhnikov, michaelh, spoorun, 
ngraham, abrahams

D16165: Don't crash on invalid exiv2 data

[Stefan Brüns]

bruns added a comment.


  In D16165#342348 , @poboiko wrote:
  
  > In D16165#342156 , @astippich 
wrote:
  >
  > > 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).

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D16165

To: poboiko, #baloo, #frameworks
Cc: bruns, astippich, kde-frameworks-devel, ashaposhnikov, michaelh, spoorun, 
ngraham, abrahams

D16165: Don't crash on invalid exiv2 data

[Igor Poboiko]

poboiko added a comment.


  In D16165#342156 , @astippich 
wrote:
  
  > I guess bugs
  >  352856 
  > 353848
  > 361259
  >  will also be fixed by this?
  
  
  Yes, they should be. Thanks for the links! Added it to summary.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D16165

To: poboiko, #baloo, #frameworks
Cc: bruns, astippich, kde-frameworks-devel, ashaposhnikov, michaelh, spoorun, 
ngraham, abrahams

D16165: Don't crash on invalid exiv2 data

[Igor Poboiko]

poboiko updated this revision to Diff 43526.
poboiko added a comment.


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

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16165?vs=43506=43526

BRANCH
  dont-crash-invalid-exiv (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D16165

AFFECTED FILES
  src/extractors/exiv2extractor.cpp

To: poboiko, #baloo, #frameworks
Cc: bruns, astippich, kde-frameworks-devel, ashaposhnikov, michaelh, spoorun, 
ngraham, abrahams

D16165: Don't crash on invalid exiv2 data

[Igor Poboiko]

poboiko edited the summary of this revision.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D16165

To: poboiko, #baloo, #frameworks
Cc: bruns, astippich, kde-frameworks-devel, ashaposhnikov, michaelh, spoorun, 
ngraham, abrahams

D16165: Don't crash on invalid exiv2 data

[Stefan Brüns]

bruns added a comment.


  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 .

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D16165

To: poboiko, #baloo, #frameworks
Cc: bruns, astippich, kde-frameworks-devel, ashaposhnikov, michaelh, spoorun, 
ngraham, abrahams

D16165: Don't crash on invalid exiv2 data

[Alexander Stippich]

astippich added a comment.


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

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D16165

To: poboiko, #baloo, #frameworks
Cc: astippich, kde-frameworks-devel, ashaposhnikov, michaelh, spoorun, ngraham, 
bruns, abrahams

D16165: Don't crash on invalid exiv2 data

[Igor Poboiko]

poboiko created this revision.
poboiko added reviewers: Baloo, Frameworks.
Herald added projects: Frameworks, Baloo.
Herald added a subscriber: kde-frameworks-devel.
poboiko requested review of this revision.

REVISION 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 with no value.
  Inside Exiv2, when we try call `toFloat()` on it, it tries to look for two 
numbers inside `value`,
  finds none and crashes.
  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)
  
  CCBUG: 375131

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

REPOSITORY
  R286 KFileMetaData

BRANCH
  dont-crash-invalid-exiv (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D16165

AFFECTED FILES
  src/extractors/exiv2extractor.cpp

To: poboiko, #baloo, #frameworks
Cc: kde-frameworks-devel, ashaposhnikov, michaelh, astippich, spoorun, ngraham, 
bruns, abrahams