<div dir="ltr">I forgot to push it earlier, pardon, here it is:<div><br></div><div>New, updated commit is here (cherry-picked the old commit and --amend -ed it with your updates, to provide a clean one for you):</div><div>
<a href="https://github.com/bostjan/shadow/commit/63f1cff362d349615e5fa4da9afb6739b6273887">https://github.com/bostjan/shadow/commit/63f1cff362d349615e5fa4da9afb6739b6273887</a><br></div><div><br></div><div>I created new branch for this one, for you to merge one single commig:</div>
<div><a href="https://github.com/bostjan/shadow/commits/feature/subuidgid-numeric-v2">https://github.com/bostjan/shadow/commits/feature/subuidgid-numeric-v2</a><br></div><div><br></div><div><br></div><div>Diff between old and new branch, for your reference:</div>
<div>(I've put free() before all returns, but not before all exits for errors, as it seems unnecessary. But, in order to avoid any more forgotten free()s maybe we should just switch this buf size to what is now specified as "Should be more than enough" and be done with it).</div>
<div><div><br></div><div><br></div><div># git diff feature/subuidgid-numeric feature/subuidgid-numeric-v2 </div><div>diff --git a/lib/subordinateio.c b/lib/subordinateio.c</div><div>index 70216a0..e5fb2f6 100644</div><div>
--- a/lib/subordinateio.c</div><div>+++ b/lib/subordinateio.c</div><div>@@ -250,9 +250,10 @@ static const struct subordinate_range *find_range(struct commonio_db *db,</div><div> exit(EXIT_FAILURE);</div><div>
}</div><div> if (owner_pwd_result == NULL) { /* Missing entry in /etc/passwd? */</div><div>+ free(buf);</div><div> return NULL;</div><div> }</div><div>- sprintf(owner_uid_string, "%d", owner_pwd.pw_uid);</div>
<div>+ sprintf(owner_uid_string, "%u", owner_pwd.pw_uid);</div><div> </div><div> </div><div> commonio_rewind(db);</div><div>@@ -291,10 +292,11 @@ static const struct subordinate_range *find_range(struct commonio_db *db,</div>
<div> </div><div> /* Owner matches, now let us check this UID/GID range */</div><div> if ((val >= first) && (val <= last)) {</div><div>+ free(buf);</div>
<div> return range;</div><div> }</div><div> }</div><div>-</div><div>+ free(buf);</div><div> return NULL;</div><div> }</div><div><br></div></div><div><br></div>
<div><br></div><div>b.</div><div><br></div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On 5 September 2014 00:45, Serge Hallyn <span dir="ltr"><<a href="mailto:serge.hallyn@ubuntu.com" target="_blank">serge.hallyn@ubuntu.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">Quoting Bostjan Skufca (<a href="mailto:bostjan@a2o.si">bostjan@a2o.si</a>):<br>
> On 1 September 2014 22:55, Serge Hallyn <<a href="mailto:serge.hallyn@ubuntu.com">serge.hallyn@ubuntu.com</a>> wrote:<br>
> > Thanks, looks good overall. Note that you are not freeing<br>
> > buf after you malloc it.<br>
> /me slightly rusty in c ATM. But hey, on the bright side I managed to<br>
> scr.w up 100% of newly used and mallocated variables, nice score!<br>
><br>
> Tnx, fixed.<br>
<br>
</div>Sorry, ISTR you hda sent a v2 of this patch to the list, but I don't see<br>
it in my mbox. If you did send it, could you please resend (I'm sorry,<br>
I'm a beast during my morning email purge)? If not, should i look at your<br>
github tree?<br>
<div><div class="h5"><br>
><br>
><br>
> > sprintf(owner_uid_string, "%d", owner_pwd.pw_uid<br>
> ><br>
> > should that be a %u ?<br>
><br>
> Yes, fixed.<br>
><br>
><br>
> > It is quite a shame as this will lengthen the failure case by<br>
> > quite a bit, especially the 'two usernames with same uid'<br>
> > concern. But it seems necessary.<br>
><br>
><br>
> Hm, it seems we have three options:<br>
> 1. single loop, as it is now<br>
> 2. dual loop, as proposed by the patch: first loop is fast, without<br>
> getpwnam syscalls, comparing literal usernames; the second is slower.<br>
> Failure case lenghtens, as you have said.<br>
> 3. single loop, where we convert and compare everything with UIDs and<br>
> ignore usernames completely.<br>
><br>
><br>
> What to do, which way to choose?<br>
><br>
> If we think about how actual login (or ssh login) process works, there<br>
> are actual usernames compared/matched and you can not use username<br>
> "uid100_user1" and then type in password from user "uid100_user2".<br>
> Therefore: usernames are compared literaly. It does not matter that<br>
> the uid is the same for both usernames.<br>
><br>
> But, once you are in the system, there the username used is of little<br>
> significance. All that matters is UID (same UID, file operation<br>
> allowed; different UID, it depends on file permissions).<br>
><br>
> I would recommend it the way the patch does it: first literal<br>
> comparison (so users are able to create preferences for different<br>
> usernames). If that fails, get the first UID that matches and use<br>
> that.<br>
><br>
><br>
> We may benchmark what actually uses more time: looping itself or<br>
> cumulated syscalls to getpwnam_r().<br>
><br>
> If it turns out that getpwnam is cheaper than looping itself, we might<br>
> as well getpwnam all entries in the first loop and save the first UID<br>
> entry that matches. If the rest of the loop does not find the exact<br>
> username, we would not have to run the loop again, as we already have<br>
> the entry of interest.<br>
<br>
</div></div>I think you're right that we should stick with what you have.<br>
<br>
thanks,<br>
-serge<br>
</blockquote></div><br></div>