Fixed a crash when browsing dirs with malformed symlinks
ClosedPublic

Authored by nmel on Feb 23 2018, 6:09 AM.

Details

Summary

Two major changes for improved stability:

  • check the status of lstat and show a "broken" file entry
  • use new readLinkSafely function which does not consider stat_p.st_size

but gradually increases buffer until the link destination path fits into
the buffer - this helps in case st_size contains garbage
(some network fs don't care about setting the right size)

As a complementary bonus, the change fixes the following warning:
filesystem.cpp:239:35: warning: variable length array ‘buffer’ is used

FIXED: [ 389413 ] Krusader crashes when entering directories with read errors
BUG: 389413
CCBUG: 390994

Test Plan

Since we can't repro the failure, please test if various symlinks (to a file, dir, another symlink) work as expected.
The bug reporter has confirmed that the patch has fixed the crash on his side.

Diff Detail

Repository
R167 Krusader
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
nmel requested review of this revision.Feb 23 2018, 6:09 AM
nmel created this revision.
nmel updated this revision to Diff 28170.Feb 27 2018, 8:27 AM
  • fixed comment
  • + readLinkSafely in place of readlink
nmel planned changes to this revision.Feb 27 2018, 8:33 AM

This is another draft for the user to test. Planning to do some appropriate refactoring if the patch works.

nmel updated this revision to Diff 28376.Mar 2 2018, 7:07 AM

improved comments, rebased on master and squashed commits

nmel retitled this revision from draft: potential fix for the bug #389413 to Fixed a crash when browsing dirs with malformed symlinks.Mar 2 2018, 7:14 AM
nmel edited the summary of this revision. (Show Details)
nmel edited the test plan for this revision. (Show Details)
nmel added a project: Krusader.
martinkostolny accepted this revision.Mar 3 2018, 3:32 PM
martinkostolny added a subscriber: martinkostolny.

Thanks, Nikita, for fixing the crash! Nice work :).

I'd like to suggest a code style of if-else to be always braced. I actually try to follow KDE coding style:
https://community.kde.org/Policies/Kdelibs_Coding_Style
(ok, that was manipulation from my side - it is "kdelibs" coding style)

But I understand if you or anybody in Krusader disagrees with that :).

This revision is now accepted and ready to land.Mar 3 2018, 3:32 PM
nmel updated this revision to Diff 28520.Mar 3 2018, 9:15 PM
nmel edited the summary of this revision. (Show Details)
  • Code style changes to conform with KDE coding style
nmel added a comment.Mar 3 2018, 9:24 PM

Martin, thanks for the review and testing, and for providing the link to the coding style. It seems kdelibs coding style is de-facto KF5 coding style. I may or may not agree with the style (actually, I even prefer opening brace on a newline), however it doesn't matter - I strongly believe that it will be extremely beneficial to use a consistent coding style across the project as the code becomes much easier to read and understand. I'll encourage others to follow this document from now on.

This revision was automatically updated to reflect the committed changes.

Oh, I didn't know about KF5 coding style, thanks! It looks indeed similar to the "kdelibs" one.

I've added the coding style link to Krusader's wiki in phabricator. Please, anyone, feel free to change the conding-style reference to anything else. I also don't want to force my personal preferences. But I also agree we should follow some common rules.