Create proper SocketAddress
AbandonedPublic

Authored by chinmoyr on Feb 3 2018, 2:04 PM.

Details

Reviewers
dfaure
ossi
Group Reviewers
Frameworks
Summary

This patch changes SocketAddress class to create an address structure for
a pathname socket (on all platforms), provide correct length of the structure
by including length of any additional implementation specific fields
and return nullptr upon address query if the address structure was not
properly initialized.

Diff Detail

Repository
R241 KIO
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
chinmoyr created this revision.Feb 3 2018, 2:04 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 3 2018, 2:04 PM
chinmoyr requested review of this revision.Feb 3 2018, 2:04 PM
ossi added inline comments.Feb 3 2018, 2:18 PM
src/ioslaves/file/fdreceiver.h
47

why are you moving the member?
it doesn't matter in this case, but generally it's better to have the bigger members first, concentrating in particular on equal size (and sizeof(QString) == sizeof(void*)).
all other things being equal, prefer no change to reduce the diff size.

src/ioslaves/file/sharefd_p.h
54

space on wrong side of amp

60

you're removing that without replacement. that makes me think that the patch is still non-atomic.

73

put spaces around binary operators

74

using strncpy() after an explicit length check is relatively pointless (unless the trailing zero padding is important).
but the operation can be optimized by using memcpy().

chinmoyr updated this revision to Diff 26436.Feb 3 2018, 2:46 PM
chinmoyr marked 3 inline comments as done.

Added space on correct side of & and on either sides of binary operators.
Used memcpy.
Restored original position of data member.

ossi added inline comments.Feb 3 2018, 2:48 PM
src/ioslaves/file/sharefd_p.h
60

you still need to address that *somehow* ;)

74

the correct length is pathSize + 1

chinmoyr marked 2 inline comments as not done.Feb 3 2018, 2:55 PM
chinmoyr added inline comments.
src/ioslaves/file/sharefd_p.h
60

I just feel like the job of SocketAddress should be to create the address structure and not perform any file operation. Removal of the socket file should be handled by file ioslave or FdReceiver.

ossi added inline comments.Feb 3 2018, 2:59 PM
src/ioslaves/file/sharefd_p.h
60

that's a valid concern, but doing that belongs into a separate patch which does the complementary change (as far as necessary) atomically, and explains it properly in the commit message.

chinmoyr updated this revision to Diff 26438.Feb 3 2018, 3:18 PM

Corrected number of bytes to be copied.
Added unlink().

ossi added inline comments.Feb 3 2018, 3:34 PM
src/ioslaves/file/kauth/fdsender.cpp
27

this actually constructs a qbytearray. that should be probably explicit.

a better approach would be moving the class' interface to qbytearray (or actually qstring) as well, to unify the API. but that's again a separate refactoring.

src/ioslaves/file/sharefd_p.h
55

you're removing the auto-prefixing with /tmp/. if that is intentional, you need to atomically adjust the callers, as otherwise you're changing behavior.

56

i now noticed that you removed that branch, thus removing the use of linux' abstract address space. that also calls for a justification.
did you research the git history to find out why it was introduced in the first place?

chinmoyr updated this revision to Diff 26441.Feb 3 2018, 3:54 PM

Used QByteArray in FdSender
Undoed behavioral changes.

chinmoyr updated this revision to Diff 26443.Feb 3 2018, 3:59 PM

Fixed typos

chinmoyr updated this revision to Diff 26445.Feb 3 2018, 4:16 PM

Check for null byte at index 1 in case of returning address length on linux.
Use size of finalPath as pathSize.

ossi added inline comments.Feb 3 2018, 5:15 PM
src/ioslaves/file/sharefd_p.h
61

you now have a buffer overflow on linux. you need to split the conditional.

chinmoyr updated this revision to Diff 26451.Feb 3 2018, 5:34 PM

Fixed buffer overflow.

thiago added a comment.Feb 3 2018, 5:40 PM

Looks good.

src/ioslaves/file/kauth/fdsender.cpp
24

The problem here is the API. Why is it using std::string in the first place?

chinmoyr added inline comments.Feb 3 2018, 5:48 PM
src/ioslaves/file/kauth/fdsender.cpp
24

The idea was to use std c++ and avoid qt throughout the class because the code will be executed with elevated privileges.

thiago added a comment.Feb 3 2018, 6:14 PM

That doesn't make sense. There's QFile::encodeName in the code.

Is there a section of the library that is used in elevated privilege code, but not all of it?

That doesn't make sense. There's QFile::encodeName in the code

I was talking talking about FdSender which is used only in kauth helper.

dfaure added inline comments.Mar 3 2018, 12:53 PM
src/ioslaves/file/kauth/fdsender.cpp
24

In that case I don't understand why SocketAddress takes a QByteArray and not a std::string.... Because ossi suggested it to unify the API and avoid one conversion to std::string in fdereceiver? But then we have two contradictory goals, we need to decide whether we use Qt or not in fdsender and therefore in SocketAddress. Since one is not supposed to use Qt API without a QCoreApplication instance, and since it's not recommented to run Qt code as root, I think your original idea made sense, no Qt in fdsender nor in SocketAddress.

ossi added inline comments.Mar 4 2018, 11:55 AM
src/ioslaves/file/kauth/fdsender.cpp
24

i can get behind the idea of keeping SocketAddress and FdSender qt-free. it's ugly that FdReceiver stands out, but it would be way harder to make that one qt-free due to its use of the event loop.

ossi requested changes to this revision.May 6 2018, 9:28 AM

note that there are also unaddressed comments from previous rounds.

src/ioslaves/file/sharefd_p.h
50

given that the linux-specific part is already gone by now, checking [1] doesn't make sense any more.

59

wait a sec, didn't an ancestor commit change that?

This revision now requires changes to proceed.May 6 2018, 9:28 AM
chinmoyr abandoned this revision.May 7 2018, 4:18 PM

Split into D12744 and D12745