D15826: [Balooshow] Avoid out-of-bounds access when accessing corrupt db data

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R293:563dd3418b60: [Balooshow] Avoid out-of-bounds access when 
accessing corrupt db data (authored by bruns).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15826?vs=44224=44524

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

AFFECTED FILES
  src/tools/balooshow/main.cpp

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

D15826: [Balooshow] Avoid out-of-bounds access when accessing corrupt db data

[Igor Poboiko]

poboiko accepted this revision.
poboiko added a comment.
This revision is now accepted and ready to land.


  Yep, fine by me

REPOSITORY
  R293 Baloo

BRANCH
  oob2

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

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

D15826: [Balooshow] Avoid out-of-bounds access when accessing corrupt db data

[Stefan Brüns]

bruns marked 3 inline comments as done.
bruns added a comment.


  @poboiko - good to go?

REPOSITORY
  R293 Baloo

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

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

D15826: [Balooshow] Avoid out-of-bounds access when accessing corrupt db data

[Stefan Brüns]

bruns marked 11 inline comments as done.

REPOSITORY
  R293 Baloo

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

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

D15826: [Balooshow] Avoid out-of-bounds access when accessing corrupt db data

[Stefan Brüns]

bruns updated this revision to Diff 44224.
bruns added a comment.


  use KLocalizedString for deferred i18n substitution

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15826?vs=43176=44224

BRANCH
  oob2

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

AFFECTED FILES
  src/tools/balooshow/main.cpp

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

D15826: [Balooshow] Avoid out-of-bounds access when accessing corrupt db data

[Igor Poboiko]

poboiko added a comment.


  I'm not entirely sure how translating system works, but won't it pop up as 4 
identical lines in i.e. `Lokalize`, causing frustration to our translators? 
  It would also mean that if we would want to change the message, we would have 
to do it in 4 different lines.
  
  I suggest introducing something like an `QString errorMessage` and printing 
it only once.

REPOSITORY
  R293 Baloo

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

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

D15826: [Balooshow] Avoid out-of-bounds access when accessing corrupt db data

[Stefan Brüns]

bruns updated this revision to Diff 43176.
bruns added a comment.


  make (introduction of) error message translatable

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15826?vs=42547=43176

BRANCH
  oob

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

AFFECTED FILES
  src/tools/balooshow/main.cpp

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

D15826: [Balooshow] Avoid out-of-bounds access when accessing corrupt db data

[Stefan Brüns]

bruns added a comment.


  In D15826#339367 , @poboiko wrote:
  
  > BTW, does it also wipe tags and other user-provided metadata?
  
  
  Tags and comments are store in XAttrs, i.e. with the file.

REPOSITORY
  R293 Baloo

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

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

D15826: [Balooshow] Avoid out-of-bounds access when accessing corrupt db data

[Igor Poboiko]

poboiko added inline comments.

INLINE COMMENTS

> bruns wrote in main.cpp:211
> but you are not allowed to access `word[2]` if `word.length() < 3`.

We don't access it directly, and `indexOf` performs internal checks. For 
example, `QStringLiteral("ab").indexOf('c', 5)` seem to be perfectly valid 
code, which returns `-1`. 
On one hand, it makes sense, and I think we can rely on it. On the other - it's 
not actually stated directly in the documentation 
...

> bruns wrote in main.cpp:218
> None of the other messages is an error message, but just regular output. Hm, 
> maybe `i18n("Internal Error: %1", message)`.

OK, I think that's better

REPOSITORY
  R293 Baloo

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

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

D15826: [Balooshow] Avoid out-of-bounds access when accessing corrupt db data

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> poboiko wrote in main.cpp:211
> But still, if the term is short (namely, length < 4), we will either won't 
> have "-" (this corresponds to `posOfNonNumeric < 0`, and that's 
> `X`), or it will be the last symbol (something like `X1-` - which is 
> `posOfNonNumeric+1 == word.length()`).

but you are not allowed to access `word[2]` if `word.length() < 3`.

> poboiko wrote in main.cpp:218
> On the one hand, you're right. On the other hand, the output should be 
> consistent. Other messages here are translated.

None of the other messages is an error message, but just regular output. Hm, 
maybe `i18n("Internal Error: %1", message)`.

REPOSITORY
  R293 Baloo

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

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

D15826: [Balooshow] Avoid out-of-bounds access when accessing corrupt db data

[Igor Poboiko]

poboiko added a comment.


  Maybe we should print also a suggestion to user, something like (maybe 
rephrase it better)
  
WARNING:
Looks like your index is corrupted.
We suggest you to run `balooctl disable && balooctl disable` to wipe it and 
rebuild from scratch
  
  so they won't have to google what to do (there's still not much can be done 
in that case)
  
  BTW, does it also wipe tags and other user-provided metadata?

INLINE COMMENTS

> bruns wrote in main.cpp:204
> See `word[0]` access directly after.
> I have fixed to many "shouldn't be" errors/crashes due to corrupt DB values 
> in the past.

OK, you're right, I guess.

> bruns wrote in main.cpp:211
> This is the exact case I have had - "X.
> The code after (`word.indexOf('-', 2)`) requires a check for length >= 3 here 
> (code correctness), semantics require >= 4.

But still, if the term is short (namely, length < 4), we will either won't have 
"-" (this corresponds to `posOfNonNumeric < 0`, and that's `X`), or it 
will be the last symbol (something like `X1-` - which is `posOfNonNumeric+1 == 
word.length()`).

> bruns wrote in main.cpp:218
> I am not really sure:
> balooctl -x is a quite low level debug tool. This is a diagnostic message 
> only printed in case of DB errors. Translated strings make search on the web 
> harder.

On the one hand, you're right. On the other hand, the output should be 
consistent. Other messages here are translated.

REPOSITORY
  R293 Baloo

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

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

D15826: [Balooshow] Avoid out-of-bounds access when accessing corrupt db data

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> poboiko wrote in main.cpp:204
> I'm not sure this check is needed - the other one (`posOfNonNumeric < 0`) 
> seem to be covering this case.
> There shouldn't be an empty `QByteArray`, right?

See `word[0]` access directly after.
I have fixed to many "shouldn't be" errors/crashes due to corrupt DB values in 
the past.

> poboiko wrote in main.cpp:211
> Same note here. The fewer code to maintain - the better :)

This is the exact case I have had - "X.
The code after (`word.indexOf('-', 2)`) requires a check for length >= 3 here 
(code correctness), semantics require >= 4.

> poboiko wrote in main.cpp:218
> I think we should `i18n()` those messages as well

I am not really sure:
balooctl -x is a quite low level debug tool. This is a diagnostic message only 
printed in case of DB errors. Translated strings make search on the web harder.

REPOSITORY
  R293 Baloo

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

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

D15826: [Balooshow] Avoid out-of-bounds access when accessing corrupt db data

[Igor Poboiko]

poboiko added a comment.


  I never experienced such corruption, though, but sanity check shouldn't hurt.

INLINE COMMENTS

> main.cpp:204
> +if (arr.length() <= 1) {
> +stream << "Malformed term (short): " << arr <<  "\n";
> +continue;

I'm not sure this check is needed - the other one (`posOfNonNumeric < 0`) seem 
to be covering this case.
There shouldn't be an empty `QByteArray`, right?

> main.cpp:211
>  if (word[0] == QLatin1Char('X')) {
> -int posOfNonNumeric = 1;
> -while (word[posOfNonNumeric] != '-') {
> -posOfNonNumeric++;
> +if (word.length() < 4) {
> +// 'X-

Same note here. The fewer code to maintain - the better :)

> main.cpp:218
> +if ((posOfNonNumeric < 0) || ((posOfNonNumeric + 1) 
> == word.length())) {
> +stream << "Malformed property term (no data): " 
> << word <<  "\n";
> +continue;

I think we should `i18n()` those messages as well

REPOSITORY
  R293 Baloo

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

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

D15826: [Balooshow] Avoid out-of-bounds access when accessing corrupt db data

[Stefan Brüns]

bruns added a reviewer: poboiko.

REPOSITORY
  R293 Baloo

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

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

D15826: [Balooshow] Avoid out-of-bounds access when accessing corrupt db data

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in main.cpp:217
> posOfNonNumeric < 0 or == -1 ?

Thou shall not code after bedtime ... Thx.

REPOSITORY
  R293 Baloo

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

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

D15826: [Balooshow] Avoid out-of-bounds access when accessing corrupt db data

[Stefan Brüns]

bruns updated this revision to Diff 42547.
bruns marked an inline comment as done.
bruns added a comment.


  fix "not found" condition for `word.indexOf(...)`

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15826?vs=42530=42547

BRANCH
  db_robustness2

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

AFFECTED FILES
  src/tools/balooshow/main.cpp

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

D15826: [Balooshow] Avoid out-of-bounds access when accessing corrupt db data

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> main.cpp:217
> +int posOfNonNumeric = word.indexOf('-', 2);
> +if ((posOfNonNumeric < -1) || ((posOfNonNumeric + 1) 
> == word.length())) {
> +stream << "Malformed property term (no data): " 
> << word <<  "\n";

posOfNonNumeric < 0 or == -1 ?

REPOSITORY
  R293 Baloo

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

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

D15826: [Balooshow] Avoid out-of-bounds access when accessing corrupt db data

[Stefan Brüns]

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

REVISION SUMMARY
  Looping over `word` without bounds check may cause illegal memory accesses,
  potentially crashing balooshow. Add sanity checks for required lengths
  and provide feedback in case an error has occured.
  
  Invalid data may occur when the DB has beend corrupted.

TEST PLAN
  corrupt database
  run `balooshow -x `

REPOSITORY
  R293 Baloo

BRANCH
  db_robustness2

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

AFFECTED FILES
  src/tools/balooshow/main.cpp

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