D10273: Create proper SocketAddress

[Chinmoy Ranjan Pradhan]

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

[Chinmoy Ranjan Pradhan]

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, thiago, dfaure, michaelh, ngraham, bruns

D10273: Create proper SocketAddress

[Oswald Buddenhagen]

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 (addr.sun_path[0] || addr.sun_path[1]) ? 
> reinterpret_cast() : nullptr;
>  }

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

> sharefd_p.h:59
> +a.sun_family = AF_UNIX;
> +const QByteArray finalPath = "/tmp/" + path;
> +const size_t pathSize = finalPath.size();

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

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

[Oswald Buddenhagen]

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 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.

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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10273

To: chinmoyr, #frameworks, dfaure
Cc: ossi, thiago, dfaure, michaelh

D10273: Create proper SocketAddress

[David Faure]

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 
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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10273

To: chinmoyr, #frameworks, dfaure
Cc: ossi, thiago, dfaure, michaelh

D10273: Create proper SocketAddress

[Luca Beltrame]

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

[Chinmoy Ranjan Pradhan]

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

[Chinmoy Ranjan Pradhan]

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
  https://phabricator.kde.org/D10273

To: chinmoyr, #frameworks
Cc: ossi, thiago, dfaure, michaelh, ngraham

D10273: Create proper SocketAddress

[Thiago Macieira]

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
Cc: ossi, thiago, dfaure, michaelh, ngraham

D10273: Create proper SocketAddress

[Chinmoy Ranjan Pradhan]

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
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10273

To: chinmoyr, #frameworks
Cc: ossi, thiago, dfaure, michaelh, ngraham

D10273: Create proper SocketAddress

[Thiago Macieira]

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
  https://phabricator.kde.org/D10273

To: chinmoyr, #frameworks
Cc: ossi, thiago, dfaure, michaelh, ngraham

D10273: Create proper SocketAddress

[Chinmoy Ranjan Pradhan]

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
  src/ioslaves/file/fdreceiver.cpp
  src/ioslaves/file/fdreceiver.h
  src/ioslaves/file/kauth/fdsender.cpp
  src/ioslaves/file/sharefd_p.h

To: chinmoyr, #frameworks
Cc: ossi, thiago, dfaure, michaelh, ngraham

D10273: Create proper SocketAddress

[Oswald Buddenhagen]

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
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10273

To: chinmoyr, #frameworks
Cc: ossi, thiago, dfaure, michaelh, ngraham

D10273: Create proper SocketAddress

[Chinmoy Ranjan Pradhan]

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

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D10273

AFFECTED FILES
  src/ioslaves/file/fdreceiver.cpp
  src/ioslaves/file/fdreceiver.h
  src/ioslaves/file/kauth/fdsender.cpp
  src/ioslaves/file/sharefd_p.h

To: chinmoyr, #frameworks
Cc: ossi, thiago, dfaure, michaelh, ngraham

D10273: Create proper SocketAddress

[Chinmoy Ranjan Pradhan]

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
  src/ioslaves/file/fdreceiver.cpp
  src/ioslaves/file/fdreceiver.h
  src/ioslaves/file/kauth/fdsender.cpp
  src/ioslaves/file/sharefd_p.h

To: chinmoyr, #frameworks
Cc: ossi, thiago, dfaure, michaelh, ngraham

D10273: Create proper SocketAddress

[Chinmoy Ranjan Pradhan]

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
  https://phabricator.kde.org/D10273

AFFECTED FILES
  src/ioslaves/file/fdreceiver.cpp
  src/ioslaves/file/fdreceiver.h
  src/ioslaves/file/kauth/fdsender.cpp
  src/ioslaves/file/kauth/fdsender.h
  src/ioslaves/file/sharefd_p.h

To: chinmoyr, #frameworks
Cc: ossi, thiago, dfaure, michaelh, ngraham

D10273: Create proper SocketAddress

[Oswald Buddenhagen]

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 qstring) as well, to unify the API. but that's again a separate 
refactoring.

> sharefd_p.h:55
> -sockaddr_un a{ AF_UNIX, {0}};
> -std::string finalPath = "/tmp/" + path;
> -#ifdef __linux__

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.

> sharefd_p.h:56
> -std::string finalPath = "/tmp/" + path;
> -#ifdef __linux__
> -::strcpy(_path[1], finalPath.c_str());

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?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10273

To: chinmoyr, #frameworks
Cc: ossi, thiago, dfaure, michaelh, ngraham

D10273: Create proper SocketAddress

[Chinmoy Ranjan Pradhan]

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
  https://phabricator.kde.org/D10273

AFFECTED FILES
  src/ioslaves/file/fdreceiver.cpp
  src/ioslaves/file/fdreceiver.h
  src/ioslaves/file/kauth/fdsender.cpp
  src/ioslaves/file/sharefd_p.h

To: chinmoyr, #frameworks
Cc: ossi, thiago, dfaure, michaelh, ngraham

D10273: Create proper SocketAddress

[Oswald Buddenhagen]

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 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10273

To: chinmoyr, #frameworks
Cc: ossi, thiago, dfaure, michaelh, ngraham

D10273: Create proper SocketAddress

[Chinmoy Ranjan Pradhan]

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. Removal of the socket file should 
be handled by file ioslave or FdReceiver.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10273

To: chinmoyr, #frameworks
Cc: ossi, thiago, dfaure, michaelh, ngraham

D10273: Create proper SocketAddress

[Oswald Buddenhagen]

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) {
> +memcpy(a.sun_path, path.constData(), sizeof(a.sun_path) - 1);
> +}

the correct length is pathSize + 1

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10273

To: chinmoyr, #frameworks
Cc: ossi, thiago, dfaure, michaelh, ngraham

D10273: Create proper SocketAddress

[Chinmoy Ranjan Pradhan]

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 UPDATE
  https://phabricator.kde.org/D10273?vs=26435=26436

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D10273

AFFECTED FILES
  src/ioslaves/file/fdreceiver.cpp
  src/ioslaves/file/fdreceiver.h
  src/ioslaves/file/kauth/fdsender.cpp
  src/ioslaves/file/sharefd_p.h

To: chinmoyr, #frameworks
Cc: ossi, thiago, dfaure, michaelh, ngraham

D10273: Create proper SocketAddress

[Oswald Buddenhagen]

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 size (and sizeof(QString) 
== sizeof(void*)).
all other things being equal, prefer no change to reduce the diff size.

> sharefd_p.h:54
>  private:
> -static sockaddr_un make_address(const std::string& path)
> +static sockaddr_un make_address(const QByteArray& path)
>  {

space on wrong side of amp

> sharefd_p.h:60
> +const size_t pathSize = path.size();
> +if (pathSize > 0 && pathSize < sizeof(a.sun_path)-1) {
> +::strncpy(a.sun_path, path.constData(), sizeof(a.sun_path)-1);

put spaces around binary operators

> sharefd_p.h:60
> -::strcpy(a.sun_path, finalPath.c_str());
> -::unlink(finalPath.c_str());
> -#endif

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

> sharefd_p.h:61
> +if (pathSize > 0 && pathSize < sizeof(a.sun_path)-1) {
> +::strncpy(a.sun_path, path.constData(), sizeof(a.sun_path)-1);
> +}

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().

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10273

To: chinmoyr, #frameworks
Cc: ossi, thiago, dfaure, michaelh, ngraham

D10273: Create proper SocketAddress

[Chinmoy Ranjan Pradhan]

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 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.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D10273

AFFECTED FILES
  src/ioslaves/file/fdreceiver.cpp
  src/ioslaves/file/fdreceiver.h
  src/ioslaves/file/kauth/fdsender.cpp
  src/ioslaves/file/sharefd_p.h

To: chinmoyr, #frameworks
Cc: ossi, thiago, dfaure, michaelh, ngraham