httpd/carddav from git sources

Ken Murchison murch at andrew.cmu.edu
Wed Jun 29 09:56:24 EDT 2016 




> On Jun 29, 2016, at 9:30 AM, Johan Hattne <johan at hattne.se> wrote:
> 
> 
>>> On Jun 29, 2016, at 09:11, Ken Murchison <murch at andrew.cmu.edu> wrote:
>>> 
>>> On Jun 29, 2016, at 8:55 AM, Johan Hattne <johan at hattne.se> wrote:
>>> 
>>> Hi Ellie & Ken;
>>> 
>>>> On Jun 28, 2016, at 20:47, ellie timoney <ellie at fastmail.com> wrote:
>>>> 
>>>> Hi Johan,
>>>> 
>>>>> In the unpatched code, a sasl_http_request_t structure is created on the
>>>>> stack and a pointer to it is copied to httpd_saslconn using
>>>>> sasl_setprop().  When the structure goes out of scope, there is no
>>>>> guarantee that its members will be preserved.  A diff against the code I
>>>>> just cloned (cyrus-imapd-3.0.0-beta2-294-ga42b500) is attached.
>>>> 
>>>> I think you're onto something here (though the attachment is missing):
>>>> 
>>>> Looking only at the cyrus-imapd code, I would have supposed that
>>>> sasl_setprop() promised to take a copy of the provided value, not just
>>>> the pointer, and that it was therefore fine for the original
>>>> sasl_http_request_t structure to go out of scope.
>>>> 
>>>> But looking at cyrus-sasl code, it looks like sasl_setprop() *generally*
>>>> takes a copy of the provided value, but for a few cases -- one of which
>>>> is SASL_HTTP_REQUEST -- it just copies the pointer.
>>>> 
>>>> I'm not sure if this is a bug in cyrus-sasl, or in cyrus-imapd, but one
>>>> of them is clearly wrong.  The man page for sasl_setprop() makes no
>>>> promises in either direction, but also doesn't document
>>>> SASL_HTTP_REQUEST at all, so the intent remains unclear.
>>>> 
>>>> Ken, what do you think?  IMO, sasl_setprop() should strictly and
>>>> consistently make its own copy of the provided value, and promise thus,
>>>> in which case the cyrus-imapd code is good.  But there might be API
>>>> change ramifications here to be wary of?
>>>> 
>>>> I'm curious, how did you come across this?
>>> 
>>> I’m not sure how to best deal with it; hence a patch (regenerated and attached this time) instead of a pull request.
>>> 
>>> I ran into this while playing around with digest authentication in carddav.  Authentication relies on the method, which is passed to libsasl in the sasl_http_request_t structure.
>> 
>> 
>> Which CardDAV client uses Digest?  Just curious. I've found out after implementing it that HTTP Digest is a mess and doesn't interoperate very well.  Last I checked Apples calendar app wasn't bumping the nonce count correctly.
> 
> I’m toying around with Apple Contacts (which appears count the total number of times the nonce goes through the wire instead of the number of times the it responds with it).  So I completely agree with Ken's assessment, but I also think that the problem in httpd is real.


Agreed.  



> 
> 
>>>>>> On Tue, Jun 28, 2016, at 05:31 PM, Johan Hattne wrote:
>>>>>> 
>>>>>> On Jun 26, 2016, at 20:02, ellie timoney <ellie at fastmail.com> wrote:
>>>>>> 
>>>>>> Hi Johan,
>>>>>> 
>>>>>> I don't know how your git repository got into that state...
>>>>>> 
>>>>>>> $ git describe
>>>>>>> cyrus-imapd-2.5-snapshot-autoconf-and-automake-3857-g4049dd2
>>>>>> 
>>>>>> This says that the nearest tag your git can find is
>>>>>> "cyrus-imapd-2.5-snapshot-autoconf-and-automake"(which is over four
>>>>>> years old), and that your branch is 3857 commits ahead of that tag (but
>>>>>> the "cyrus-imapd-2.5.0" release tag is only 1796 commits ahead, so why
>>>>>> didn't describe find that, instead?), and that the commit id of your
>>>>>> branch tip is "4049dd2" (which I don't have, presumably because it's
>>>>>> your local commit containing your changes).
>>>>>> 
>>>>>> You probably want to refetch entirely I suspect [but keep reading first]
>>>>>> 
>>>>>>> * remote origin
>>>>>>> Fetch URL: git://git.cyrusimap.org/cyrus-imapd
>>>>>>> Push  URL: git://git.cyrusimap.org/cyrus-imapd
>>>>>>> HEAD branch: master
>>>>>> 
>>>>>> I think these URLs are wrong/old.  If you can even fetch from them
>>>>>> anymore, do they update?  Look at the dates on the recent commits -- are
>>>>>> they ancient?  ('git log --format=fuller' to see commit dates).
>>>>>> 
>>>>>> But anyway, we just moved our repositories to github last week, so
>>>>>> anything you get from your old origin is going to be stale now
>>>>>> regardless.  https://github.com/cyrusimap/cyrus-imapd is the github page
>>>>>> with the clone/fork/etc links.
>>>>> 
>>>>> Thanks a lot, Ellie;
>>>>> 
>>>>> I’m confused about the repositories; I didn’t commit anything and the
>>>>> last log entries are from August 2015.  Nevertheless, I cloned from
>>>>> GitHub as per above and imap-makefile.patch is indeed obsolete.  However,
>>>>> I find that the httpd patch, or something addressing the same symptom, is
>>>>> still necessary.
>>>>> 
>>>>> In the unpatched code, a sasl_http_request_t structure is created on the
>>>>> stack and a pointer to it is copied to httpd_saslconn using
>>>>> sasl_setprop().  When the structure goes out of scope, there is no
>>>>> guarantee that its members will be preserved.  A diff against the code I
>>>>> just cloned (cyrus-imapd-3.0.0-beta2-294-ga42b500) is attached.
>>>>> 
>>>>>>> I got this all from
>>>>>>> https://cyrusimap.org/mediawiki/index.php/Contribute#Anonymous_GIT_Access;
>>>>>>> the text indicates that development is actually happening elsewhere, but
>>>>>>> it has never been clear to me what the relationship between the
>>>>>>> repositories is.
>>>>>> 
>>>>>> Okay, that mediawiki is ancient -- I actually thought it was gone
>>>>>> already -- so that explains the old info.   I guess you got there from
>>>>>> Google, because it hasn't been linked from cyrusimap.org for ages.
>>>>>> 
>>>>>> Anyway, I suspect once you set up the correct git remote, and fetch from
>>>>>> that, your build troubles will go away.
>>>>> 
>>>>> Yes, I think I got to the mediawiki via Google.  
>>>>> 
>>>>> // Best wishes; Johan
>>>>> 
>>>>> 
>>>>>>> On Fri, Jun 24, 2016, at 11:33 PM, Johan Hattne wrote:
>>>>>>> Hi Ellie;
>>>>>>> 
>>>>>>> $ git remote show origin | head -n 4
>>>>>>> * remote origin
>>>>>>> Fetch URL: git://git.cyrusimap.org/cyrus-imapd
>>>>>>> Push  URL: git://git.cyrusimap.org/cyrus-imapd
>>>>>>> HEAD branch: master
>>>>>>> $ git describe
>>>>>>> cyrus-imapd-2.5-snapshot-autoconf-and-automake-3857-g4049dd2
>>>>>>> 
>>>>>>> I got this all from
>>>>>>> https://cyrusimap.org/mediawiki/index.php/Contribute#Anonymous_GIT_Access;
>>>>>>> the text indicates that development is actually happening elsewhere, but
>>>>>>> it has never been clear to me what the relationship between the
>>>>>>> repositories is.
>>>>>>> 
>>>>>>> // Best wishes; Johan
>>>>>>> 
>>>>>>>> On Jun 23, 2016, at 19:08, ellie timoney via Info-cyrus <info-cyrus at lists.andrew.cmu.edu> wrote:
>>>>>>>> 
>>>>>>>> Hi Johan,
>>>>>>>> 
>>>>>>>> What revision are these patches against?
>>>>>>>> 
>>>>>>>> The Makefile.am patch is unnecessary, and doesn't apply cleanly anyway,
>>>>>>>> so I suspect you're looking at an old revision.
>>>>>>>> 
>>>>>>>> I haven't studied the httpd patch in depth yet.
>>>>>>>> 
>>>>>>>> Cheers,
>>>>>>>> 
>>>>>>>> ellie
>>>>>>>> 
>>>>>>>>> On Thu, Jun 23, 2016, at 12:59 AM, Johan Hattne via Info-cyrus wrote:
>>>>>>>>> Dear all;
>>>>>>>>> 
>>>>>>>>> To make carddav run from git sources
>>>>>>>>> (git://git.cyrusimap.org/cyrus-imapd) I had to apply attached tiny
>>>>>>>>> patches; the Makefile.am patch addresses a use-before-definition issue
>>>>>>>>> which causes automake-1.14.1 to fall over, the httpd patch ensures that
>>>>>>>>> the sasl_http_request_t structure which is set as a property of
>>>>>>>>> httpd_saslconn does not go out of scope before it is used (I suppose this
>>>>>>>>> behavior is a tad system-dependent).  Apologies if both these issues have
>>>>>>>>> already been addressed elsewhere.
>>>>>>>>> 
>>>>>>>>> // Best wishes; Johan
>>>>>>>>> 
>>>>>>>>> ----
>>>>>>>>> Cyrus Home Page: http://www.cyrusimap.org/
>>>>>>>>> List Archives/Info: http://lists.andrew.cmu.edu/pipermail/info-cyrus/
>>>>>>>>> To Unsubscribe:
>>>>>>>>> https://lists.andrew.cmu.edu/mailman/listinfo/info-cyrus
>>>>>>>>> Email had 2 attachments:
>>>>>>>>> + imap-httpd.patch
>>>>>>>>> 1k (text/plain)
>>>>>>>>> + imap-makefile.patch
>>>>>>>>> 1k (text/plain)
>>>>>>>> ----
>>>>>>>> Cyrus Home Page: http://www.cyrusimap.org/
>>>>>>>> List Archives/Info: http://lists.andrew.cmu.edu/pipermail/info-cyrus/
>>>>>>>> To Unsubscribe:
>>>>>>>> https://lists.andrew.cmu.edu/mailman/listinfo/info-cyrus
>>> <imap-httpd.patch>
>> 
>> --
>> Kenneth Murchison
>> Principal Systems Software Engineer
>> Carnegie Mellon University
>

More information about the Info-cyrus mailing list