<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/129526/">https://git.reviewboard.kde.org/r/129526/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 8th, 2017, 4:09 p.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/129526/diff/1/?file=486385#file486385line422" style="color: black; font-weight: bold; text-decoration: underline;">pam_kwallet.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">418</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">snprintf</span><span class="p">(</span><span class="n">fullSocket</span><span class="p">,</span> <span class="n">needed</span><span class="p">,</span> <span class="s">"%s/%s%s"</span><span class="p">,</span> <span class="n">socketPath</span><span class="p">,</span> <span class="n">socketPrefix</span><span class="p">,</span> <span class="s">".socket"</span><span class="p">);</span><span class="ew"> </span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">trailing spaces</p></pre>
</blockquote>
<p>On January 8th, 2017, 4:49 p.m. UTC, <b>Damjan Georgievski</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">according to http://standards.freedesktop.org/basedir-spec/latest/, one is supposed to check permissions</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't see it in the specs, and it says: „The directory MUST be owned by the user, and he MUST be the only one having read and write access to it. Its Unix access mode MUST be 0700.“ - but it might be a sensible thing to check (although there are race conditions in checking and only trying to use it later).</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">trailing spaces</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">ughh, what do I do now, "Update diff"?</p></pre>
</blockquote>
<p>On January 8th, 2017, 5:26 p.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, these "MUST" are exactly what I meant the code is supposed to check before using XDG_RUNTIME_DIR :)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm confused by your reply, you say "I don't see it in the spec" and then you quote exactly what I am referring to.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There is no race condition in checking for "I own it and it's 0700" before using, because this can only change if root intervenes, another user cannot do anything about a dir that he doesn't own and that is 0700. And if root is compromised, all is lost anyway ;)</p></pre>
</blockquote>
<p>On January 8th, 2017, 5:47 p.m. UTC, <b>Damjan Georgievski</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm confused by your reply, you say "I don't see it in the spec" and then you quote exactly what I am referring to.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">huh, I might be mistaken, but the way I read it, the creator of $XDG_RUNTIME_DIR MUST do those things, otherwise it shouldn't set the environment variable.</p></pre>
</blockquote>
<p>On January 8th, 2017, 6:13 p.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I see. It's indeed unclear in the spec. I guess it was clearer in the initial discussion on the ML...</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Every one else checks though ;)
https://git.enlightenment.org/core/enlightenment.git/commit/?id=2520c73d04049f1c84c67cc5e3e55c10d2078025
https://github.com/whitequark/rust-xdg/pull/10/files
+ what I did in QStandardPaths.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Anyway, compared to /tmp I guess a world-writable socket dir isn't worse, I'm not going to veto it since the spec is unclear, I'll let someone else approve this though. Meanwhile I'll ask for clarification on the xdg list ;)</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Lennart clarified this on the xdg list:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"When I wrote this I always had in mind that the component setting
XDG_RUNTIME_DIR is responsible for preparating the dir the right way,
and that apps may simply trust that the dir is properly set up when
they see the environment variable set.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That said, people do weird stuff with su/sudo. It might or might not
make sense for apps to superficially check ownership of the dir before
using it. However I am very sure apps should never try to "fix" it it
doesn't match their expectations, as that most likely would make
things worse, not better in such su/sudo setups."</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So I'm OK with no checks.</p></pre>
<br />
<p>- David</p>
<br />
<p>On January 8th, 2017, 4:59 p.m. UTC, Damjan Georgievski wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks.</div>
<div>By Damjan Georgievski.</div>
<p style="color: grey;"><i>Updated Jan. 8, 2017, 4:59 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=365722">365722</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kwallet-pam
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Most recent Linux distributions setup a per-user XDG_RUNTIME_DIR as a tmpfs, which is also tied to their session lifecycle. Typically this is in /run/user/1000/.
My suggestion is to use $XDG_RUNTIME_DIR/kwallet5.socket if XDG_RUNTIME_DIR exists, or fallback to /tmp/kwallet5_${username}.socket if it doesn't.
Reproducible: Always</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>pam_kwallet.c <span style="color: grey">(809ab9a)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/129526/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>