Hi Ellie & Ken;
> On Jun 28, 2016, at 20:47, ellie timoney <ellie@xxxxxxxxxxxx> 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.
// Cheers; Johan
> On Tue, Jun 28, 2016, at 05:31 PM, Johan Hattne wrote:
>>
>>> On Jun 26, 2016, at 20:02, ellie timoney <ellie@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx> 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
>>>>
>>
Attachment:
imap-httpd.patch
Description: Binary data
---- 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
