Your Cyrus imapd ACL patch

Tue Aug 16 11:43:39 EDT 2011

On Monday, August 15, 2011 05:14:02 AM Greg Banks wrote:
> I just tried again, and saw the same thing.
> 
> The Cyrus version was the cmu master branch at commit
> 4412656e218a42559964ccdce06e8daefb8197c5 which is Guilherme's patch,
> plus a5caf503c7060b4f1ec546e4dc6fe75e5b9c4029 to enable the tests to run.
> 
> My Valgrind is version 1:3.6.0~svn20100724-0ubuntu2 on Ubuntu 10.10, run

Hi Greg, 

I built the cyrus master branch at commit 
4412656e218a42559964ccdce06e8daefb8197c5 which is Guilherme's patch (with 
a5caf503c7060b4f1ec546e4dc6fe75e5b9c4029 applied, but did not use cassandane), 
and built valgrind-3.6.0 fetching the original source from 
http://valgrind.org/downloads/valgrind-3.6.0.tar.bz2 with no patches (I used a 
patched version of 3.6.1 before). But I was still not able to reproduce the 
same valgrind output, it is really annoying (started cyrus with "valgrind --
tool=memcheck --verbose --leak-check=full --trace-children=yes --track-
origins=yes --show-reachable=yes --undef-value-errors=yes 
/usr/cyrus/bin/cyrus-master" this time). Hmm, wonder what could be the 
difference between our systems... I am testing under a custom built linux 
system now. 

So I added some logging to the imap/mboxlist.c:mboxlist_is_owner function 
(patch attached: cyrus-imapd-mboxlist_is_owner-logging.patch). I created a 
user "user.base" and executed a "setacl user.base admin lrswipkxtecda" command 
with both the IMAP commands you provided via telnet and cyradm manually 
(deleted "user.base" inbetween of course). Both produced the same output. 

Here are some log extracts about it:

*** Testing with telnet/imap ***

telnet localhost 143
Trying 127.0.0.1...
Connected to intradevel-aiesec.net.lan (127.0.0.1).
Escape character is '^]'.
* OK [CAPABILITY IMAP4rev1 LITERAL+ ID ENABLE STARTTLS AUTH=GSSAPI AUTH=LOGIN 
AUTH=CRAM-MD5 AUTH=DIGEST-MD5 AUTH=PLAIN SASL-IR] intradevel-aiesec.net.lan 
server ready
. login admin *****
. OK [CAPABILITY IMAP4rev1 LITERAL+ ID ENABLE ACL RIGHTS=kxten QUOTA MAILBOX-
REFERRALS NAMESPACE UIDPLUS NO_ATOMIC_RENAME UNSELECT CHILDREN MULTIAPPEND 
BINARY CATENATE CONDSTORE ESEARCH SORT SORT=MODSEQ SORT=DISPLAY 
THREAD=ORDEREDSUBJECT THREAD=REFERENCES ANNOTATEMORE METADATA LIST-EXTENDED 
LIST-STATUS WITHIN QRESYNC SCAN XLIST SPECIAL-USE CREATE-SPECIAL-USE URLAUTH 
URLAUTH=BINARY X-NETSCAPE LOGINDISABLED COMPRESS=DEFLATE IDLE] User logged in 
SESSIONID=<intradevel-aiesec.net.lan-2705-1313501882-1>
. create user.base
. OK Completed
. setacl user.base admin lrswipkxtecda
. OK Completed
. logout
* BYE LOGOUT received
. OK Completed
Connection closed by foreign host.

[!] calling mboxlist_is_owner, arguments: name: user^base domainlen: 0 user: 
admin userlen: 5 [!]
[!] name+domainlen: user^base, enough_length: 0 [!]
[!] cp: (null), user_complete: 1, (cp - user: -73713080) [!]
[!] name+domainlen+5: base , match_strings: 0 [!]
[!] name[domainlen+5+userlen]: #000, match_length: 1 [!]
[!] Returning from mboxlist_is_owner, return value: 0 [!]

[!] calling mboxlist_is_owner, arguments: name: user^base domainlen: 0 user: 
admin userlen: 5 [!]
[!] name+domainlen: user^base, enough_length: 0 [!]
[!] cp: (null), user_complete: 1, (cp - user: -73726664) [!]
[!] name+domainlen+5: base , match_strings: 0 [!]
[!] name[domainlen+5+userlen]: #000, match_length: 1 [!]
[!] Returning from mboxlist_is_owner, return value: 0 [!]

*** Testing with cyradm ***

cyradm localhost --user admin
Password: 
intradevel-aiesec.net.lan> cm user.base
intradevel-aiesec.net.lan> setacl user.base admin lrswipkxtecda
intradevel-aiesec.net.lan> quit

[!] calling mboxlist_is_owner, arguments: name: user^base domainlen: 0 user: 
admin userlen: 5 [!]
[!] name+domainlen: user^base, enough_length: 0 [!]
[!] cp: (null), user_complete: 1, (cp - user: -73714152) [!]
[!] name+domainlen+5: base , match_strings: 0 [!]
[!] name[domainlen+5+userlen]: #000, match_length: 1 [!]
[!] Returning from mboxlist_is_owner, return value: 0 [!]

[!] calling mboxlist_is_owner, arguments: name: user^base domainlen: 0 user: 
admin userlen: 5 [!]
[!] name+domainlen: user^base, enough_length: 0 [!]
[!] cp: (null), user_complete: 1, (cp - user: -73727496) [!]
[!] name+domainlen+5: base , match_strings: 0 [!]
[!] name[domainlen+5+userlen]: #000, match_length: 1 [!]
[!] Returning from mboxlist_is_owner, return value: 0 [!]

This line in the mboxlist_is_owner function must be wrong, looking at the 
logs: 
"int enough_length = !strncmp(name+domainlen, "user.", 5);"
It should be "int enough_length = !strncmp(name+domainlen, "user^", 5);", I 
think. Or even better using the macro defined at imap/mboxname.h:53:
#define DOTCHAR '^', I guess. 

The indexing here is also odd: name[domainlen+5+userlen], that is name[10] in 
the case above and name is "user^base". The name buffer may be some fixed size 
buffer, the code does not crash here.

I got the info, that there is a previous version of this ACLs patch somewhere, 
there the mboxlist_is_owner function's structure is a bit different, the values 
of the variables "enough_length, user_complete, match_strings and 
match_length" are calculated in order, but a later one is not calculated if 
the previous one is false (it should be some big if statement).

Anyway, thanks for your feedback; I'll try to tidy this up, something is not 
entirely OK here. 

Kristóf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cyrus-imapd-mboxlist_is_owner-logging.patch
Type: text/x-patch
Size: 1486 bytes
Desc: not available
Url : http://lists.andrew.cmu.edu/pipermail/cyrus-devel/attachments/20110816/7a15108f/attachment.bin 