Sent from my iPhone
On 17/08/2011, at 1:43, Kristóf Katus <[email protected]>
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.
