<!DOCTYPE html>
<html>
<head>
<title></title>
</head>
<body><div style="font-family:Arial;">The problem is entirely that there's no search_time_t_cmp or similar for time values. That and the fact that these things operate on pointers to memory that doesn't have enough type information to ensure we aren't doing something stupid.<br></div>
<div style="font-family:Arial;"><br></div>
<div style="font-family:Arial;">We have a problem with time_t that many of the values we're comparing against (internaldate, sentdate, etc) are stored in 32 bit fields in cyrus.index, which is probably why the 32 bit function was chosen.<br></div>
<div style="font-family:Arial;"><br></div>
<div style="font-family:Arial;">Bron.<br></div>
<div><br></div>
<div><br></div>
<div>On Wed, 21 Sep 2016, at 12:43, ellie timoney via Cyrus-devel wrote:<br></div>
<blockquote type="cite"><div>Hi Richard,<br></div>
<div><br></div>
<div>Thanks for bumping this, looks like it got forgotten about. I've put this into the github issues tracker as <a href="https://github.com/cyrusimap/cyrus-imapd/issues/27">https://github.com/cyrusimap/cyrus-imapd/issues/27</a> so at least it won't get forgotten again.<br></div>
<div><br></div>
<div>It looks from your patch like "internaldate" and "sentdate" are the specific fields causing problems? (Based on line numbers, since the patch doesn't have *quite* enough useful context, doh!)<br></div>
<div><br></div>
<div>We do already have autoconf testing for sizeof time_t -- e.g. my config.h (x86_64) gets:<br></div>
<div><br></div>
<div>> /* The size of `time_t', as computed by sizeof. */<br></div>
<div>> #define SIZEOF_TIME_T 8<br></div>
<div><br></div>
<div>... but not much code actually looks for that. And my thinking is the same as yours: trivial to patch in this one spot, but what else needs doing?<br></div>
<div><br></div>
<div>I don't know search well/at all, so maybe someone who does can chime in in more detail here. Otherwise, I'll experiment and read up a bit and see if I can reproduce/understand it.<br></div>
<div><br></div>
<div>Cheers,<br></div>
<div><br></div>
<div>ellie<br></div>
<div><br></div>
<div>On Tue, Sep 20, 2016, at 11:51 PM, Richard Garnish via Cyrus-devel wrote:<br></div>
<blockquote type="cite"><div>Since sending this report I haven't seen any correspondence on the
issue mentioned. Has the discrepancy of time_t size been addressed
in some way yet? A glance at search_expr.c in git suggests not,
unless the problem has been attacked from the other direction by
forcing time_t to 32-bit.<br></div>
<div><br></div>
<div>Richard<br></div>
<div><br></div>
<div><div><br></div>
<div>-------- Forwarded Message --------<br></div>
<table border="0" cellpadding="0" cellspacing="0"><tbody><tr><th nowrap="nowrap">Subject:<br></th><td>Stack corruption on platform with 64-bit time_t<br></td></tr><tr><th nowrap="nowrap">Date:<br></th><td>Fri, 29 Apr 2016 20:32:42 +0100<br></td></tr><tr><th nowrap="nowrap">From:<br></th><td>Richard Garnish <a href="mailto:richard@cinesite.com"><richard@cinesite.com></a><br></td></tr><tr><th nowrap="nowrap">To:<br></th><td><a href="mailto:cyrus-devel@lists.andrew.cmu.edu">cyrus-devel@lists.andrew.cmu.edu</a><br></td></tr></tbody></table><div><br></div>
<div><br></div>
<pre>I've been testing 3.0.0-beta2 on CentOS 7.2 for deployment, and have
come across a crash on date searches. For example, many Android clients
will start by requesting a list of UIDs from the last 2 weeks:
1601 Select {5+}
INBOX
* 152 EXISTS
* 151 RECENT
* FLAGS (\Answered \Flagged \Draft \Deleted \Seen NonJunk)
* OK [PERMANENTFLAGS (\Answered \Flagged \Draft \Deleted \Seen NonJunk
\*)] Ok
* OK [UNSEEN 1] Ok
* OK [UIDVALIDITY 1461950888] Ok
* OK [UIDNEXT 154] Ok
* OK [HIGHESTMODSEQ 6] Ok
* OK [URLMECH INTERNAL] Ok
* OK [ANNOTATIONS 65536] Ok
1601 OK [READ-WRITE] Completed
1602 UID Search all since 15-Apr-2016
*** stack smashing detected ***: imapd terminated
======= Backtrace: =========
/usr/lib64/libc.so.6(__fortify_fail+0x37)[0x7fbf13e6ab37]
/usr/lib64/libc.so.6(__fortify_fail+0x0)[0x7fbf13e6ab00]
/usr/lib64/libcyrus_imap.so.0(+0x6bbc6)[0x7fbf166a5bc6]
/usr/lib64/libcyrus_imap.so.0(search_expr_evaluate+0x16c)[0x7fbf166a848c]
/usr/lib64/libcyrus_imap.so.0(search_expr_evaluate+0x4c)[0x7fbf166a836c]
/usr/lib64/libcyrus_imap.so.0(+0x38d64)[0x7fbf16672d64]
/usr/lib64/libcyrus_imap.so.0(+0x6f3b8)[0x7fbf166a93b8]
/usr/lib64/libcyrus_imap.so.0(+0x6f566)[0x7fbf166a9566]
/usr/lib64/libcyrus_imap.so.0(search_query_run+0x72)[0x7fbf166a9f82]
/usr/lib64/libcyrus_imap.so.0(index_search+0x69)[0x7fbf16675899]
imapd(+0x2849b)[0x7fbf16d6349b]
imapd(+0x2a9e8)[0x7fbf16d659e8]
imapd(+0x1a699)[0x7fbf16d55699]
/usr/lib64/libc.so.6(__libc_start_main+0xf5)[0x7fbf13d7eb15]
imapd(+0x1ada9)[0x7fbf16d55da9]
The crash immediate follows search_uint32_cmp operating on
m->record.internaldate (of type time_t.) On CentOS 7 64-bit, time_t
(and size_t, off_t, long) are 64-bit types by default. Because it's
Friday evening, I've quickly fixed the problem by replacing the search
functions for "internaldate" and "sentdate" with the 64-bit equivalents
in attrs[] within search_attr_init() and this resolves the crash
observed above.
However, I'm sure the right way to handle this properly is to test for
sizeof(time_t) in autoconf and select the search functions accordingly.
I'm also unsure whether this type mismatch impacts anything else -- for
instance cyrus.index uses 32-bit fields for dates, and mailbox.c feeds
internaldate, sentdate, gmtime and last_updated through htonl() and
ntohl() to populate these fields, without explicit typecasting.
Richard
--- cyrus-imapd-3.0.0-beta2/imap/search_expr.c.orig 2016-04-29
19:19:23.453052354 +0100
+++ cyrus-imapd-3.0.0-beta2/imap/search_expr.c 2016-04-29
19:20:20.074296701 +0100
@@ -2277,10 +2277,10 @@ EXPORTED void search_attr_init(void)
SEARCH_PART_NONE,
SEARCH_COST_INDEX,
/*internalise*/NULL,
- search_uint32_cmp,
- search_uint32_match,
- search_uint32_serialise,
- search_uint32_unserialise,
+ search_uint64_cmp,
+ search_uint64_match,
+ search_uint64_serialise,
+ search_uint64_unserialise,
/*get_countability*/NULL,
/*duplicate*/NULL,
/*free*/NULL,
@@ -2291,10 +2291,10 @@ EXPORTED void search_attr_init(void)
SEARCH_PART_NONE,
SEARCH_COST_INDEX,
/*internalise*/NULL,
- search_uint32_cmp,
- search_uint32_match,
- search_uint32_serialise,
- search_uint32_unserialise,
+ search_uint64_cmp,
+ search_uint64_match,
+ search_uint64_serialise,
+ search_uint64_unserialise,
/*get_countability*/NULL,
/*duplicate*/NULL,
/*free*/NULL,
--
Richard Garnish
Senior Systems Administrator
Cinesite VFX Limited
<br></pre></div>
</blockquote><div><br></div>
</blockquote><div style="font-family:Arial;"><br></div>
<div id="sig567075"><div class="signature">--<br></div>
<div class="signature"> Bron Gondwana<br></div>
<div class="signature"> brong@fastmail.fm<br></div>
<div class="signature"><br></div>
</div>
<div style="font-family:Arial;"><br></div>
</body>
</html>