D8806: Do not leak rfkill file descriptors.

[David Edmundson]

davidedmundson added a comment.


  Not to spam this review request:
  
  > We're still looking for someone to take the time to properly benchmark 
that, the last time this happened was 10 years ago at least.
  
  FWIW: I did a very very basic test recently.
  I put Q_BENCHMARK round QProcess, and using kinit via KToolInvocation to 
launch dolphin. Dolphin at least links kinit, not sure if it really works. 
  Then used KWindowSystem to wait for a window to be added, which seemed a 
real-world test of "readiness".
  
  Results were near identical. If anything via KToolInvocation was slower, but 
it had an extra internal KService lookup so not a completely fair test.

REPOSITORY
  R269 BluezQt

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

To: ofreyermuth, davidedmundson
Cc: dfaure, drosca, bshah, broulik, #frameworks

D8806: Do not leak rfkill file descriptors.

[David Faure]

dfaure added a comment.


  ... because this isn't kde4 anymore, where almost every app provided a 
kdeinit module.
  
  QProcess forks from the process that uses QProcess, it might look 
inconsistent when looking at process trees but it's really the most standard 
Unix way of executing processes: forking ;)
  kdeinit was the non-standard way...
  
  For dependency reasons, not everything provides a kdeinit module anymore.
  It's also very unclear still whether kdeinit brings any performance benefits 
in 2017.
  We're still looking for someone to take the time to properly benchmark that, 
the last time this happened was 10 years ago at least.

REPOSITORY
  R269 BluezQt

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

To: ofreyermuth, davidedmundson
Cc: dfaure, drosca, bshah, broulik, #frameworks

D8806: Do not leak rfkill file descriptors.

[David Rosca]

drosca added a comment.


  Though on the other hand, why isn't KLauncher (KRun) used for running apps 
everywhere? This way they are forked from `kdeinit5` and this issue is 
non-existant.
  
  Quick test:
  
  - Launch from taskmanager -> forked from plasmashell
  - Launch from quicklaunch applet -> forked from kdeinit
  - Launch from krunner -> forked from ksmserver
  - Launch from global shortcut -> forked from kded

REPOSITORY
  R269 BluezQt

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

To: ofreyermuth, davidedmundson
Cc: drosca, bshah, broulik, #frameworks

D8806: Do not leak rfkill file descriptors.

[Bhushan Shah]

bshah added a comment.


  In https://phabricator.kde.org/D8806#167510, @drosca wrote:
  
  > Why? It's not like any other process can't open /dev/rfkill as we ship udev 
rule to enable r+w access to /dev/rfkill for all users.
  
  
  My comment was question :-)
  
  But well I didn't know about udev rule.. in that case it's fine

REPOSITORY
  R269 BluezQt

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

To: ofreyermuth, davidedmundson
Cc: drosca, bshah, broulik, #frameworks

D8806: Do not leak rfkill file descriptors.

[David Rosca]

drosca added a comment.


  In https://phabricator.kde.org/D8806#167509, @bshah wrote:
  
  > I do wonder if this is security fix? And should probably be handled like 
that?
  
  
  Why? It's not like any other process can't open /dev/rfkill as we ship udev 
rule to enable r+w access to /dev/rfkill for all users.

REPOSITORY
  R269 BluezQt

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

To: ofreyermuth, davidedmundson
Cc: drosca, bshah, broulik, #frameworks

D8806: Do not leak rfkill file descriptors.

[Bhushan Shah]

bshah added a comment.


  I do wonder if this is security fix? And should probably be handled like that?

REPOSITORY
  R269 BluezQt

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

To: ofreyermuth, davidedmundson
Cc: drosca, bshah, broulik, #frameworks

D8806: Do not leak rfkill file descriptors.

[David Rosca]

This revision was automatically updated to reflect the committed changes.
Closed by commit R269:ae044d6dfec9: Do not leak rfkill file descriptors. 
(authored by ofreyermuth, committed by drosca).

REPOSITORY
  R269 BluezQt

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D8806?vs=22299=22316

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

AFFECTED FILES
  src/rfkill.cpp

To: ofreyermuth, davidedmundson
Cc: drosca, bshah, broulik, #frameworks

D8806: Do not leak rfkill file descriptors.

[Oliver Freyermuth]

ofreyermuth added a comment.


  In https://phabricator.kde.org/D8806#167503, @drosca wrote:
  
  > I need your full name and e-mail to commit it (I can't get it with `arch 
patch` probably because you didn't upload this review with `arc diff`).
  
  
  Oliver Freyermuth 
  
  Didn't know about `arc` just yet, but will use it for future work, thanks for 
the hint!

REPOSITORY
  R269 BluezQt

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

To: ofreyermuth, davidedmundson
Cc: drosca, bshah, broulik, #frameworks

D8806: Do not leak rfkill file descriptors.

[David Rosca]

drosca added a comment.


  Alright, thanks for the explanation.
  
  I need your full name and e-mail to commit it (I can't get it with `arch 
patch` probably because you didn't upload this review with `arc diff`).

REPOSITORY
  R269 BluezQt

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

To: ofreyermuth, davidedmundson
Cc: drosca, bshah, broulik, #frameworks

D8806: Do not leak rfkill file descriptors.

[Oliver Freyermuth]

ofreyermuth added a comment.


  In https://phabricator.kde.org/D8806#167483, @drosca wrote:
  
  > Just for the record, how does Konsole inherit this fd when BluezQt is only 
used in plasmashell + kded and KDE apps are afaik not forked from these 
processes?
  
  
  What exactly happens depends on how Konsole is started.
  If it's started via plasmashell (e.g. menu), the process tree is:
  
kdeinit5: Running...
 \_ /usr/lib64/libexec/kf5/klauncher --fd=9
 \_ kded5 [kdeinit5]
 \_ /usr/bin/ksmserver
 |   \_ /usr/bin/kwin_x11 -session 
10cf9da06500015028345170087800055_1510626492_975085
 |   \_ /usr/bin/plasmashell
 |   |   \_ /usr/bin/konsole
 |   |   \_ /bin/zsh
 |   |   \_ ps faux
  
  If you register a custom shortcut to start a terminal and launch Konsole via 
that, you end up with:
  
kdeinit5: Running...
 \_ /usr/lib64/libexec/kf5/klauncher --fd=9
 \_ kded5 [kdeinit5]
 |   \_ /usr/bin/konsole
 |   \_ /bin/zsh
 |   \_ ps faux
 \_ /usr/bin/ksmserver
  
  So it's either forked from plasmashell or kded on my system at least.
  
  The issue has also been observed before: 
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=860269
  but sadly the fix got lost in the Debian bugtracker.

REPOSITORY
  R269 BluezQt

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

To: ofreyermuth, davidedmundson
Cc: drosca, bshah, broulik, #frameworks

D8806: Do not leak rfkill file descriptors.

[Oliver Freyermuth]

ofreyermuth added a comment.


  In https://phabricator.kde.org/D8806#167457, @bshah wrote:
  
  > Do you have commit access?
  
  
  In case you are adressing me: No.

REPOSITORY
  R269 BluezQt

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

To: ofreyermuth, davidedmundson
Cc: drosca, bshah, broulik, #frameworks

D8806: Do not leak rfkill file descriptors.

[David Rosca]

drosca added a comment.


  Just for the record, how does Konsole inherit this fd when BluezQt is only 
used in plasmashell + kded and KDE apps are afaik not forked from these 
processes?

REPOSITORY
  R269 BluezQt

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

To: ofreyermuth, davidedmundson
Cc: drosca, bshah, broulik, #frameworks

D8806: Do not leak rfkill file descriptors.

[Bhushan Shah]

bshah added a comment.


  Do you have commit access?

REPOSITORY
  R269 BluezQt

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

To: ofreyermuth, davidedmundson
Cc: bshah, broulik, #frameworks

D8806: Do not leak rfkill file descriptors.

[Oliver Freyermuth]

ofreyermuth added a comment.


  In https://phabricator.kde.org/D8806#167404, @broulik wrote:
  
  > If you add the following (upper-case with a colon and space) on its own 
line in your commit message [...]
  
  
  Many thanks, done!

REPOSITORY
  R269 BluezQt

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

To: ofreyermuth, davidedmundson
Cc: broulik, #frameworks

D8806: Do not leak rfkill file descriptors.

[Oliver Freyermuth]

ofreyermuth edited the summary of this revision.

REPOSITORY
  R269 BluezQt

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

To: ofreyermuth, davidedmundson
Cc: broulik, #frameworks

D8806: Do not leak rfkill file descriptors.

[Kai Uwe Broulik]

broulik added a comment.


  If you add the following (upper-case with a colon and space) on its own line 
in your commit message the bug will be automatically closed once it landed:
  
BUG: 386886

REPOSITORY
  R269 BluezQt

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

To: ofreyermuth, davidedmundson
Cc: broulik, #frameworks

D8806: Do not leak rfkill file descriptors.

[Oliver Freyermuth]

ofreyermuth added a comment.


  Thanks for the quick review!
  I also created a related issue report here:
  https://bugs.kde.org/show_bug.cgi?id=386886
  which can of course be closed once the patch has landed.

REPOSITORY
  R269 BluezQt

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

To: ofreyermuth, davidedmundson
Cc: #frameworks

D8806: Do not leak rfkill file descriptors.

[David Edmundson]

davidedmundson accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R269 BluezQt

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

To: ofreyermuth, davidedmundson
Cc: #frameworks

D8806: Do not leak rfkill file descriptors.

[Oliver Freyermuth]

ofreyermuth created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.

REVISION SUMMARY
  They may be leaked into all child processes,
  including regular Konsole terminals on KDE.
  
  I observed them showing up at all processes in my KDE session, just checking
  ls -la /proc/self/fd
  
  Using O_CLOEXEC, things are resolved properly.

REPOSITORY
  R269 BluezQt

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

AFFECTED FILES
  src/rfkill.cpp

To: ofreyermuth
Cc: #frameworks