D10273: Create proper SocketAddress

chinmoyr removed a dependent revision: D10409: In linux don't use abstract socket to share file descriptor. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10273 To: chinmoyr, #frameworks, dfaure, ossi Cc: ossi, thiago, dfaure, michaelh, ngraham, bruns

D10273: Create proper SocketAddress

chinmoyr abandoned this revision. chinmoyr added a comment. Split into D12744 and D12745 REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10273 To: chinmoyr, #frameworks, dfaure, ossi Cc: ossi,

D10273: Create proper SocketAddress

ossi requested changes to this revision. ossi added a comment. This revision now requires changes to proceed. note that there are also unaddressed comments from previous rounds. INLINE COMMENTS > sharefd_p.h:50 > { > -return reinterpret_cast(); > +return

D10273: Create proper SocketAddress

ossi added inline comments. INLINE COMMENTS > dfaure wrote in 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

D10273: Create proper SocketAddress

dfaure added inline comments. INLINE COMMENTS > chinmoyr wrote in 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. In that case I don't understand why SocketAddress takes a QByteArray and not a

D10273: Create proper SocketAddress

lbeltrame added a reviewer: dfaure. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10273 To: chinmoyr, #frameworks, dfaure Cc: ossi, thiago, dfaure, michaelh

D10273: Create proper SocketAddress

chinmoyr added a dependent revision: D10409: In linux don't use abstract socket to share file descriptor. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10273 To: chinmoyr, #frameworks Cc: ossi, thiago, dfaure, michaelh, ngraham

D10273: Create proper SocketAddress

chinmoyr added a comment. In https://phabricator.kde.org/D10273#200254, @thiago wrote: > 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. REPOSITORY R241 KIO REVISION DETAIL

D10273: Create proper SocketAddress

thiago added a comment. 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? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10273 To: chinmoyr, #frameworks

D10273: Create proper SocketAddress

chinmoyr added inline comments. INLINE COMMENTS > thiago wrote in fdsender.cpp:24 > The problem here is the API. Why is it using std::string in the first place? The idea was to use std c++ and avoid qt throughout the class because the code will be executed with elevated privileges. REPOSITORY

D10273: Create proper SocketAddress

thiago added a comment. Looks good. INLINE COMMENTS > fdsender.cpp:24 > > FdSender::FdSender(const std::string ) > : m_socketDes(-1) The problem here is the API. Why is it using std::string in the first place? REPOSITORY R241 KIO REVISION DETAIL

D10273: Create proper SocketAddress

chinmoyr updated this revision to Diff 26451. chinmoyr added a comment. Fixed buffer overflow. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10273?vs=26445=26451 BRANCH master REVISION DETAIL https://phabricator.kde.org/D10273 AFFECTED FILES

D10273: Create proper SocketAddress

ossi added inline comments. INLINE COMMENTS > sharefd_p.h:61 > +const size_t pathSize = finalPath.size(); > +if (pathSize > 5 && pathSize < sizeof(a.sun_path) - 1) { > #ifdef __linux__ you now have a buffer overflow on linux. you need to split the conditional. REPOSITORY

D10273: Create proper SocketAddress

chinmoyr updated this revision to Diff 26445. chinmoyr added a comment. Check for null byte at index 1 in case of returning address length on linux. Use size of finalPath as pathSize. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10273?vs=26443=26445

D10273: Create proper SocketAddress

chinmoyr updated this revision to Diff 26443. chinmoyr added a comment. Fixed typos REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10273?vs=26441=26443 BRANCH master REVISION DETAIL https://phabricator.kde.org/D10273 AFFECTED FILES

D10273: Create proper SocketAddress

chinmoyr updated this revision to Diff 26441. chinmoyr added a comment. Used QByteArray in FdSender Undoed behavioral changes. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10273?vs=26438=26441 BRANCH master REVISION DETAIL

D10273: Create proper SocketAddress

ossi added inline comments. INLINE COMMENTS > fdsender.cpp:27 > { > +SocketAddress addr(path.c_str()); > +if (!addr.address()) { this actually constructs a qbytearray. that should be probably explicit. a better approach would be moving the class' interface to qbytearray (or actually

D10273: Create proper SocketAddress

chinmoyr updated this revision to Diff 26438. chinmoyr added a comment. Corrected number of bytes to be copied. Added unlink(). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D10273?vs=26436=26438 BRANCH master REVISION DETAIL

D10273: Create proper SocketAddress

ossi added inline comments. INLINE COMMENTS > chinmoyr wrote in 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. that's a

D10273: Create proper SocketAddress

chinmoyr marked 2 inline comments as not done. chinmoyr added inline comments. INLINE COMMENTS > ossi wrote in sharefd_p.h:60 > you still need to address that *somehow* ;) I just feel like the job of SocketAddress should be to create the address structure and not perform any file operation.

D10273: Create proper SocketAddress

ossi added inline comments. INLINE COMMENTS > sharefd_p.h:60 > -::strcpy(a.sun_path, finalPath.c_str()); > -::unlink(finalPath.c_str()); > -#endif you still need to address that *somehow* ;) > sharefd_p.h:61 > +if (pathSize > 0 && pathSize < sizeof(a.sun_path) - 1) { >

D10273: Create proper SocketAddress

chinmoyr updated this revision to Diff 26436. chinmoyr marked 3 inline comments as done. chinmoyr added a comment. Added space on correct side of & and on either sides of binary operators. Used memcpy. Restored original position of data member. REPOSITORY R241 KIO CHANGES SINCE LAST

D10273: Create proper SocketAddress

ossi added inline comments. INLINE COMMENTS > fdreceiver.h:45 > +QString m_path; > +QSocketNotifier *m_readNotifier; > }; 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

D10273: Create proper SocketAddress

chinmoyr created this revision. chinmoyr added a reviewer: Frameworks. Restricted Application added a project: Frameworks. chinmoyr requested review of this revision. REVISION SUMMARY This patch changes SocketAddress class to create an address structure for a pathname socket (on all