Differential D16770

[ftp kio-slave] Fix deletion of directory with non-latin1/utf8 parent path
AbandonedPublic
Actions

Authored by kossebau on Nov 9 2018, 12:53 AM.

Details

Reviewers
dfaure
Summary

KRemoteEncoding::directory() returns the encoded path of the directpry,
while Ftp::ftpFolder() takes the un-encoded version.
The implicit casting from QByteArray via QString::fromUtf8 to the QString
type for the Ftp::ftpFolder() arg prevented the compiler from complaining about
that coding mismatch.
This principle code issue might have been hidden all the years due to non-latin1
characters being rare with ftp server directory layouts. More, most ftp servers
are using UTF-8 encoding , so the implicit cast via QString::fromUtf8 by
accident was matching what the explicit cast via remoteEncoding()->decode()
in such cases is doing.

This patch makes the code correct and also working for any theoretical ftp
servers which use a non-latin1/utf8 encoding.

Test Plan

Tested with am ftp server which uses UTF-8 encoding, deleting folders
works as before.

Diff Detail

Repository
R241 KIO
Branch
fixFtpDirdelete
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 4746
Build 4764: arc lint + arc unit
kossebau created this revision.Nov 9 2018, 12:53 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptNov 9 2018, 12:53 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
kossebau requested review of this revision.Nov 9 2018, 12:53 AM
kossebau retitled this revision from [ftp kio-slave] Fix deletion of directory with non-latin parent path to [ftp kio-slave] Fix deletion of directory with non-latin1/utf8 parent path.Nov 10 2018, 12:52 AM
kossebau edited the summary of this revision. (Show Details)
kossebau edited the test plan for this revision. (Show Details)
kossebau added a subscriber: aacid.

@aacid Thanks for providing your ftp server, served its purpose.

dfaure requested changes to this revision.Nov 10 2018, 9:44 PM
dfaure added inline comments.
src/ioslaves/ftp/ftp.cpp
1240

directory() encodes, and then you call decode(). Sounds slow and convoluted.

I would write it like this instead:

const QString parentDir = url.adjusted(QUrl::StripTrailingSlash).adjusted(QUrl::RemoveFilename).path();
This revision now requires changes to proceed.Nov 10 2018, 9:44 PM
kossebau abandoned this revision.Nov 29 2019, 6:29 PM

Solved by D24349 meanwhile in the same way.

dfaure added a comment.Nov 30 2019, 12:32 PM

Hmmpf, indeed. I forgot about the objection I had here :)

Are you interested in testing my suggested fix, since you seemed to have a testcase for this code?

kossebau added a comment.EditedNov 30 2019, 12:44 PM
In D16770#569723, @dfaure wrote:

Hmmpf, indeed. I forgot about the objection I had here :)

Are you interested in testing my suggested fix, since you seemed to have a testcase for this code?

Been some time, and forgot details, also was using some ftp servers I have no longer access to.
Yet I remember is that I preferred the current variant for some reason, but not which, and was collecting enough arguments for a convincing reply, that's why I had not instantly replied then. Perhaps it was something about not trusting QUrl to do the right thing in all cases, but really no more memories.

Edit: perhaps it was something about non-utf8 encodings on either side where QUrl then might miss something. but I would have first to dig up things again, and ftp is too dead in my world to draw resources to that :)

Revision Contents
Changeset List

PathPackages
Msrc/ioslaves/ftp/ftp.cpp (3 lines)

Diff 45148

View Options

src/ioslaves/ftp/ftp.cpp

Loading...