[SSSD] [PATCH 6/6] Fix segfault in GSSAPI reconnect code

Eugene Indenbom eindenbom at gmail.com
Fri May 7 06:33:52 UTC 2010


Dear Stephen and Dmitri,

We are going in rounds. I have already addressed all this copy-paste 
hell in my patches concerning GSSAPI reconnect.

I would appreciate very much if you look into my patches now and give me 
feedback. The patches can be found at:
https://fedorahosted.org/pipermail/sssd-devel/2010-April/003113.html

0003-Fixed-recursive-sdap_handle-disconnect-sequence-from.patch is 
already superseded by Simo's clever cancel-destruction solution and can 
be dropped, but other patches are still valid.

0004-The-LDAP-ID-backend-connection-logic-has-been-refact.patch actually 
addresses multitude of problems in LDAP request processing including 
copy-paste hell you are trying to fix now.

All patches apply on top off SSSD-1.1.1. If you find them useful as a 
base for further discussion, I will rebase them to sssd-1.2 or master on 
your choice.

I have also posted tickets for all issues resolved in my patch as 
promised earlier.

Eugene Indenbom


On 05/07/2010 02:33 AM, Dmitri Pal wrote:
> Stephen Gallagher wrote:
>    
>> On 05/06/2010 02:48 PM, Stephen Gallagher wrote:
>>      
>>>>>> 1) The code seems to be duplicated multiple times. Why common code is
>>>>>> not segmented into a common function?
>>>>>>              
>>>>> It is now :)
>>>>>            
>> Dmitri pointed out to me off-list that there was a much larger section
>> of redundant code I could turn into a common function.
>>
>> New patch attached.
>>
>>      
> Sorry but let me be a bit picky
> +    enum account_restart *err* = SDAP_ACCOUNT_DO_NOT_RESTART;
> +
> +    if (ret != EOK) {
> +        *dp_err = DP_ERR_FATAL;
> +
> +        if (ret == ETIMEDOUT || ret == EFAULT || ret == EIO) {
> +            *dp_err = DP_ERR_OFFLINE;
> +            ctx =
> talloc_get_type(breq->be_ctx->bet_info[BET_ID].pvt_bet_data,
> +                                  struct sdap_id_ctx);
> +            if (sdap_check_gssapi_reconnect(ctx)) {
> +                if (ctx->gsh) {
> +                    /* Mark the connection as false so we don't try to
> use an
> +                     * invalid connection by mistake later.
> +                     * If the global sdap handler is NULL, it's ok not
> to do
> +                     * anything here. It's always checked by
> sdap_connected()
> +                     * before being used.
> +                     */
> +                    ctx->gsh->connected = false;
> +                }
> +                *err* = sdap_account_info_restart(breq);
> +                if (*err* == *EOK*) return SDAP_ACCOUNT_DO_RESTART;
>
>
> how about the following approach:
>
>
>
>
> +void sdap_account_info_common_done(int ret, struct be_req *breq, const
> char *errstr)
> +{
> +    struct sdap_id_ctx *ctx;
> +    int err = DP_ERR_OK;
> +    int error = EOK;
> +
> +    if (ret != EOK) {
> +        err = DP_ERR_FATAL;
> +
> +        if (ret == ETIMEDOUT || ret == EFAULT || ret == EIO) {
> +            err = DP_ERR_OFFLINE;
> +            ctx =
> talloc_get_type(breq->be_ctx->bet_info[BET_ID].pvt_bet_data,
> +                                  struct sdap_id_ctx);
> +            if (sdap_check_gssapi_reconnect(ctx)) {
> +                if (ctx->gsh) {
> +                    /* Mark the connection as false so we don't try to
> use an
> +                     * invalid connection by mistake later.
> +                     * If the global sdap handler is NULL, it's ok not
> to do
> +                     * anything here. It's always checked by
> sdap_connected()
> +                     * before being used.
> +                     */
> +                    ctx->gsh->connected = false;
> +                }
> +                error = sdap_account_info_restart(breq);
> +                if (!error) return;
> +            }
> +            sdap_mark_offline(ctx);
> +        }
> +        sdap_handler_done(breq, err, ret, errstr);
> +    }
> +    else sdap_handler_done(breq, DP_ERR_OK, EOK, NULL);
> +}
> +
>   static void sdap_account_info_users_done(struct tevent_req *req)
>   {
>       struct be_req *breq = tevent_req_callback_data(req, struct be_req);
> -    struct sdap_id_ctx *ctx;
>       int dp_err = DP_ERR_OK;
> -    const char *error = NULL;
> -    int ret, err;
> +    const char *errstr = "Enum Users Failed";
> +    int ret;
> +    enum account_restart err;
>
>       ret = users_get_recv(req);
>       talloc_zfree(req);
>
> -    if (ret) {
> -        dp_err = DP_ERR_FATAL;
> -        error = "Enum Users Failed";
> -
> -        if (ret == ETIMEDOUT || ret == EFAULT || ret == EIO) {
> -            dp_err = DP_ERR_OFFLINE;
> -            ctx =
> talloc_get_type(breq->be_ctx->bet_info[BET_ID].pvt_bet_data,
> -                                  struct sdap_id_ctx);
> -            if (sdap_check_gssapi_reconnect(ctx)) {
> -                ctx->gsh->connected = false;
> -                err = sdap_account_info_restart(breq);
> -                if (err == EOK) return;
> -            }
> -            sdap_mark_offline(ctx);
> -        }
> -    }
> -    sdap_handler_done(breq, dp_err, ret, error);
> +    sdap_account_info_common_done(ret, breq, errstr);
>
>   }
>
> And then you do not need enum that looks a bit ugly here...
>
> At least this gives the idea of what I mean...
>    




More information about the sssd-devel mailing list