<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta http-equiv="content-type" content="text/html; charset=ISO-8859-1">
</head>
<body bgcolor="#ffffff" text="#000000">
G'day,<br>
<br>
Resending to the list in hopes of reaching Guilherme; the email
address in the commit message is bouncing.<br>
<br>
-------- Original Message --------
<table class="moz-email-headers-table" border="0" cellpadding="0"
cellspacing="0">
<tbody>
<tr>
<th align="RIGHT" nowrap="nowrap" valign="BASELINE">Subject: </th>
<td>Your Cyrus imapd ACL patch</td>
</tr>
<tr>
<th align="RIGHT" nowrap="nowrap" valign="BASELINE">Date: </th>
<td>Tue, 09 Aug 2011 16:47:23 +1000</td>
</tr>
<tr>
<th align="RIGHT" nowrap="nowrap" valign="BASELINE">From: </th>
<td>Greg Banks <a class="moz-txt-link-rfc2396E" href="mailto:gnb@fastmail.fm"><gnb@fastmail.fm></a></td>
</tr>
<tr>
<th align="RIGHT" nowrap="nowrap" valign="BASELINE">To: </th>
<td>Guilherme Maciel Ferreira
<a class="moz-txt-link-rfc2396E" href="mailto:guilherme.maciel.ferreira@intra2net.com"><guilherme.maciel.ferreira@intra2net.com></a></td>
</tr>
<tr>
<th align="RIGHT" nowrap="nowrap" valign="BASELINE">CC: </th>
<td>Jeroen van Meeuwen (Kolab Systems)
<a class="moz-txt-link-rfc2396E" href="mailto:vanmeeuwen@kolabsys.com"><vanmeeuwen@kolabsys.com></a></td>
</tr>
</tbody>
</table>
<br>
<br>
<pre>G'day,
I've been using the Cassandane test suite to test a version of Cyrus
imapd containing your patch
<a class="moz-txt-link-freetext" href="http://git.cyrusimap.org/cyrus-imapd/commit/?id=4412656e218a42559964ccdce06e8daefb8197c5">http://git.cyrusimap.org/cyrus-imapd/commit/?id=4412656e218a42559964ccdce06e8daefb8197c5</a>
(cc'ing Jeroen who committed the patch) and I'm seeing the following
Valgrind errors
Conditional jump or move depends on uninitialised value(s)
at 0x45056E: mboxlist_is_owner (mboxlist.c:1354)
by 0x450A3D: mboxlist_setacl (mboxlist.c:1451)
by 0x41DEFB: cmd_setacl (imapd.c:7188)
by 0x40DA48: cmdloop (imapd.c:1915)
by 0x40A928: service_main (imapd.c:970)
by 0x4094CD: main (service.c:570)
Conditional jump or move depends on uninitialised value(s)
at 0x450584: mboxlist_is_owner (mboxlist.c:1354)
by 0x450A3D: mboxlist_setacl (mboxlist.c:1451)
by 0x41DEFB: cmd_setacl (imapd.c:7188)
by 0x40DA48: cmdloop (imapd.c:1915)
by 0x40A928: service_main (imapd.c:970)
by 0x4094CD: main (service.c:570)
where
1346 static int mboxlist_is_owner(const char *name, int domainlen,
1347 const char *user, int userlen)
1348 {
1349 char *cp = NULL;
1350
1351 int enough_length = !strncmp(name+domainlen, "user.", 5);
1352 int user_complete = (!(cp = strchr(user, '.')) || (cp - user)> userlen);
1353 int match_strings = !strncmp(name+domainlen+5, user, userlen);
1354 int match_length = (name[domainlen+5+userlen] == '\0' ||<---------
1355 name[domainlen+5+userlen] == '.');
1356
1357 return (enough_length&& user_complete&& match_strings&& match_length);
1358 }
and
1449 /* checks if the mailbox belongs to the user who is trying to change the
1450 access rights */
1451 if (mboxlist_is_owner(name, domainlen, userid, useridlen)) {
1452 isusermbox = 1;
1453 }
1454 anyoneuseracl = config_getswitch(IMAPOPT_ANYONEUSERACL);
1455
1456 /* checks if the identifier is the mailbox owner */
1457 if (mboxlist_is_owner(name, domainlen, identifier, identifierlen)) {<-------
1458 isidentifiermbox = 1;
1459 }
1460
1461 /* who is the mailbox owner? */
1462 if (isusermbox) {
1463 mailbox_owner = userid;
1464 }
1465 else if (isidentifiermbox) {
1466 mailbox_owner = identifier;
1467 }
I can't quite work out what the code in mboxlist_setacl() thinks it's
supposed to be doing. The variable 'identifierlen' is initially set to
strlen(identifier) but then the variable 'identifier' is reassigned on
some paths without changing 'identifierlen', and then both 'identifier'
and 'identifierlen' are passed to mboxlist_is_owner() which uses one to
index the other. Something is clearly broken but I can't quite tell what.
To help your debugging, I instrumented imapd to list the arguments to
cmd_setacl() to the Valgrind log:
**29768** cmd_setacl(tag="3" name="user.cassandane" identifier="admin" rights="lrswipkxtecda") imapd_userid="admin"
**29768** cmd_setacl(tag="4" name="user.cassandane" identifier="cassandane" rights="lrswipkxtecd") imapd_userid="admin"
**29768** cmd_setacl(tag="5" name="user.cassandane" identifier="anyone" rights="p") imapd_userid="admin"
**29768** cmd_setacl(tag="5" name="user.base" identifier="admin" rights="lrswipkxtecda") imapd_userid="admin"
==29768== Conditional jump or move depends on uninitialised value(s)
==29768== at 0x45082E: mboxlist_is_owner (mboxlist.c:1354)
==29768== by 0x450CFD: mboxlist_setacl (mboxlist.c:1451)
==29768== by 0x41E1B9: cmd_setacl (imapd.c:7192)
==29768== by 0x40DCC6: cmdloop (imapd.c:1916)
==29768== by 0x40ABA6: service_main (imapd.c:971)
==29768== by 0x4094CD: main (service.c:570)
==29768==
==29768== Conditional jump or move depends on uninitialised value(s)
==29768== at 0x450844: mboxlist_is_owner (mboxlist.c:1354)
==29768== by 0x450CFD: mboxlist_setacl (mboxlist.c:1451)
==29768== by 0x41E1B9: cmd_setacl (imapd.c:7192)
==29768== by 0x40DCC6: cmdloop (imapd.c:1916)
==29768== by 0x40ABA6: service_main (imapd.c:971)
==29768== by 0x4094CD: main (service.c:570)
==29768==
==29768== Conditional jump or move depends on uninitialised value(s)
==29768== at 0x45082E: mboxlist_is_owner (mboxlist.c:1354)
==29768== by 0x450D3F: mboxlist_setacl (mboxlist.c:1457)
==29768== by 0x41E1B9: cmd_setacl (imapd.c:7192)
==29768== by 0x40DCC6: cmdloop (imapd.c:1916)
==29768== by 0x40ABA6: service_main (imapd.c:971)
==29768== by 0x4094CD: main (service.c:570)
==29768==
==29768== Conditional jump or move depends on uninitialised value(s)
==29768== at 0x450844: mboxlist_is_owner (mboxlist.c:1354)
==29768== by 0x450D3F: mboxlist_setacl (mboxlist.c:1457)
==29768== by 0x41E1B9: cmd_setacl (imapd.c:7192)
==29768== by 0x40DCC6: cmdloop (imapd.c:1916)
==29768== by 0x40ABA6: service_main (imapd.c:971)
==29768== by 0x4094CD: main (service.c:570)
==29768==
**29768** cmd_setacl(tag="6" name="user.base.subdir2" identifier="admin" rights="lrswipkxtecda") imapd_userid="admin"
**29768** cmd_setacl(tag="26" name="user.baseplus" identifier="admin" rights="lrswipkxtecda") imapd_userid="admin"
**29768** cmd_setacl(tag="27" name="user.baseplus.subdir" identifier="admin" rights="lrswipkxtecda") imapd_userid="admin"
Hmm, it looks like perhaps mboxlist_is_owner() is walking off the end of
the 'name' string when the folder name is user.foo and the length of
'foo' is less than the length of the current logged in user.
Any chance you could fix this?
--
Greg.
</pre>
</body>
</html>