D9966: [KIO] Fix issues with sharing of file descriptor

[Fabian Vogt]

fvogt requested changes to this revision.
fvogt added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> fdreceiver.cpp:42
> +        std::cerr << "Invalid socket address" << std::endl;
> +        return;
> +    }

Missing

  ::close(m_socketDes);
  m_socketDes = -1;

or just move it above the `m_socketDes` assignment.

> fdreceiver.cpp:60
>      }
> +    ::unlink(m_path.toStdString().c_str());
>  }

A quick question: Is it possible that there are two FdReceivers with the same pid?
In that case they would end up removing each other's sockets.

> file_unix.cpp:91
> +    // value and reset it after deleting socket file.
> +    int err = errno;
> +    QFile::remove(sockPath);

That looks like a hack. If errno is that important, save and keep it in a variable. Every call into a library can change errno, it's extremely volatile.

> sharefd_p.h:53
>      }
> +    bool error() const
> +    {

Just a suggestion: Remove `error()` and just return `nullptr` in `address()` in the error case.
That way it's not possible to use it accidentially.

> sharefd_p.h:66
> +            ::strncpy(a.sun_path, path.c_str(), sizeof(a.sun_path)-1);
> +        }
>          return a;

The socket permissions only work that way for linux, so if other OSs can use this code as well it's insecure and should `#error` out.

REPOSITORY
  R241 KIO

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

To: chinmoyr, #frameworks, thiago, fvogt
Cc: ngraham, fvogt, lbeltrame, dfaure
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180123/4c500748/attachment.html>