Your Cyrus imapd ACL patch
gnb at fastmail.fm
Tue Aug 16 19:49:34 EDT 2011
Sent from my iPhone
On 17/08/2011, at 1:43, Kristóf Katus <kristof.katus at intra2net.com>
> 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
> a5caf503c7060b4f1ec546e4dc6fe75e5b9c4029 applied, but did not use
> 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 --
> 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
> system now.
> So I added some logging to the imap/mboxlist.c:mboxlist_is_owner
> (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
> (deleted "user.base" inbetween of course). Both produced the same
> 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=CRAM-MD5 AUTH=DIGEST-MD5 AUTH=PLAIN SASL-IR] intradevel-
> server ready
> . login admin *****
> . OK [CAPABILITY IMAP4rev1 LITERAL+ ID ENABLE ACL RIGHTS=kxten QUOTA
> REFERRALS NAMESPACE UIDPLUS NO_ATOMIC_RENAME UNSELECT CHILDREN
> BINARY CATENATE CONDSTORE ESEARCH SORT SORT=MODSEQ SORT=DISPLAY
> THREAD=ORDEREDSUBJECT THREAD=REFERENCES ANNOTATEMORE METADATA LIST-
> LIST-STATUS WITHIN QRESYNC SCAN XLIST SPECIAL-USE CREATE-SPECIAL-USE
> URLAUTH=BINARY X-NETSCAPE LOGINDISABLED COMPRESS=DEFLATE IDLE] User
> logged in
> . 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
> "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 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
> 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.
More information about the Cyrus-devel