man ioslave: asserts trying to display pam(8)
ClosedPublic

Authored by marten on Nov 28 2017, 1:00 PM.

Details

Summary

That page, and many others, links to another page with just a plain name:

.so PAM.8

This is read by man2html and passed to MANProtocol::readManPage() with a filename of "../PAM.8". This is then resolved against the containing directory "/usr/share/man/man8", becoming "/usr/share/man/PAM.8". No such file exists and the mandir.entryList().first() asserts because the list is empty.

The relative filename is correct in the case of other pages. e.g. telinit(8) which uses:

.so man8/init.8

So it is not enough to simply ignore the "..". The change to man2html here attempts to correct the reference by detecting whether it is of the form "../page.sect" and is not "../manN/page.sect", and removing the ".." component only in the first case.

While investigating this I found it useful to have a better diagnostic if the .so page could not be found. The change here to kio_man.cpp detects that case and displays an error page instead of asserting. This should only happen if there is a bug in the man page, but it is useful to have a pointer so that the upstream man page can be fixed.

Test Plan

Built kio_extras with this change. Verified correct display of the man pages:

man:/pam(8) - simple page name
man:/service(8)
man:/telinit(8) - relative page name with section
man:/vigr(8)
man:/mq_open(2) - relative page name to another section

Diff Detail

Repository
R320 KIO Extras
Lint
Lint Skipped
Unit
Unit Tests Skipped
marten created this revision.Nov 28 2017, 1:00 PM
Restricted Application added a project: Plasma. · View Herald TranscriptNov 28 2017, 1:00 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
marten requested review of this revision.Nov 28 2017, 1:00 PM
apol added a subscriber: apol.Nov 28 2017, 5:23 PM

Lack of context in the patch makes it hard to review.

man/kio_man.cpp
570

mark const.

marten updated this revision to Diff 23118.Nov 29 2017, 8:37 AM

Used const where appropriate.

Apologies for the lack of context in the diff - I hadn't realised until you pointed it out that Phabricator doesn't automatically provide context (unlike Reviewboard). Maybe there should be a mention in https://community.kde.org/Infrastructure/Phabricator about generating the diff with a suitable amount of context?

marten marked an inline comment as done.Nov 29 2017, 8:37 AM
marten added a comment.Jan 8 2018, 1:20 PM

Ping anyone - is the updated diff enough for review?

Restricted Application added projects: Dolphin, Frameworks. · View Herald TranscriptMar 1 2019, 10:17 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript

Apologize for the long delay. It looks like that the fix still applies and works according the comment.

I added @mkoller, the historical maintainer of kio-man, who can probably validate the change better than me, but let's try to push this for the upcoming KDE Applications 19.04.

mkoller accepted this revision.Jul 18 2019, 11:02 AM
This revision is now accepted and ready to land.Jul 18 2019, 11:02 AM
This revision was automatically updated to reflect the committed changes.