Your Cyrus imapd ACL patch

[Greg Banks]

Sent from my iPhone

On 17/08/2011, at 1:43, Kristóf Katus <kristof.katus at intra2net.com>  
wrote:

> 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).

I don't use --trace-children, although I don't see why that should  
matter. Are you getting any log messages from Valgrind running in the  
child imapd process at all?


> 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 [!]

Well there's a difference, I see "user.base" not "user^base". Sounds  
like there's some difference between our imapd.confs. Although by this  
point I would expect the name to be internalised.

>
>
> 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).

That sounds better. Another option would be returning early from the  
function. Or using mboxname_to_parts() to do the parsing instead.

Greg.
>