[PATCH] LDAP: Conditional jump depends on uninitialised value

[Lukas Slebodnik]

ehlo,

It haven't caused any problem yet because it is low probability that
ret != EOK && has_posix == false

"Usually, undefined value means non-zero value"

simple patch is attached.

LS

[Sumit Bose]

On Mon, Jan 26, 2015 at 06:31:05PM +0100, Lukas Slebodnik wrote:
> ehlo,
>
> It haven't caused any problem yet because it is low probability that
> ret != EOK && has_posix == false
>
> "Usually, undefined value means non-zero value"
>
> simple patch is attached.
>
> LS
> From 094d48fa6163d2286a20664b81b7b955e56bfe16 Mon Sep 17 00:00:00 2001
> From: Lukas Slebodnik 
> Date: Mon, 26 Jan 2015 15:43:08 +0100
> Subject: [PATCH] LDAP: Conditional jump depends on uninitialised value
>
> ==31767==    at 0x5B66CFC: users_get_posix_check_done (ldap_id.c:346)
> ==31767==    by 0x4DCC6AA: _tevent_req_notify_callback (tevent_req.c:112)
> ==31767==    by 0x4DCC724: tevent_req_finish (tevent_req.c:149)
> ==31767==    by 0x4DCC782: _tevent_req_error (tevent_req.c:167)
> ==31767==    by 0x5B7ED43: sdap_posix_check_done (sdap_async.c:2486)
> ==31767==    by 0x4DCC6AA: _tevent_req_notify_callback (tevent_req.c:112)
> ==31767==    by 0x4DCC724: tevent_req_finish (tevent_req.c:149)
> ==31767==    by 0x4DCC782: _tevent_req_error (tevent_req.c:167)
> ==31767==    by 0x5B7DE37: sdap_get_generic_op_finished (sdap_async.c:1523)
> ==31767==    by 0x5B7D62B: sdap_process_result (sdap_async.c:357)
> ==31767==    by 0x4DCFC1C: tevent_common_loop_timer_delay (tevent_timed.c:341)
> ==31767==    by 0x4DD0E12: epoll_event_loop_once (tevent_epoll.c:911)
> ==31767==    by 0x4DCF23E: std_event_loop_once (tevent_standard.c:114)
> ==31767==    by 0x4DCB38F: _tevent_loop_once (tevent.c:530)
> ==31767==    by 0x4DCB58B: tevent_common_loop_wait (tevent.c:634)
> ==31767==    by 0x4DCF1BE: std_event_loop_wait (tevent_standard.c:140)
> ==31767==    by 0x4DCB627: _tevent_loop_wait (tevent.c:653)
> ==31767==    by 0x489AB98: server_loop (server.c:668)
> ==31767==    by 0x10D035: main (data_provider_be.c:2915)
>
> src/providers/ldap/ldap_id.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/providers/ldap/ldap_id.c b/src/providers/ldap/ldap_id.c
> index 2e58f4e49eb33a85cbb8b4144c69004c6b5b312b..affbdad957087515961177c2378d82d52bc1ef90 100644
> --- a/src/providers/ldap/ldap_id.c
> +++ b/src/providers/ldap/ldap_id.c
> @@ -343,7 +343,7 @@ static void users_get_posix_check_done(struct tevent_req *subreq)
>  /* If the check ran to completion, we know for certain about the attributes
>   */
> if (has_posix == false) {
> if (ret == EOK && has_posix == false) {
>     state->sdap_ret = ERR_NO_POSIX;
>     tevent_req_done(req);
>     return;

I think there is still a chance that has_posix is uninitialised.
After ret != EOK is returned in 325 we jump in the if-block starting at
331. If sdap_id_op_done() finishes successful has_posix is still
undefined but ret == EOK in line 346.

325     ret = sdap_posix_check_recv(subreq, &has_posix);
 326     talloc_zfree(subreq);
 327     if (ret != EOK) {
 328         /* We can only finish the id_op on error as the connection
 329          * is re-used by the user search
 330          */
 331         ret = sdap_id_op_done(state->op, ret, &dp_error);
 332         if (dp_error == DP_ERR_OK && ret != EOK) {
 333             /* retry */
 334             ret = users_get_retry(req);
 335             if (ret != EOK) {
 336                 tevent_req_error(req, ret);
 337             }
 338             return;
 339         }
 340     }
 341 
 342     state->ctx->srv_opts->posix_checked = true;
 343 
 344     /* If the check ran to completion, we know for certain about the attributes
 345      */
 346     if (has_posix == false) {
 347         state->sdap_ret = ERR_NO_POSIX;
 348         tevent_req_done(req);
 349         return;
 350     }

bye,
Sumit
> -- 
> 2.1.0
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Lukas Slebodnik]

On (26/01/15 20:14), Sumit Bose wrote:
> I think there is still a chance that has_posix is uninitialised.
> After ret != EOK is returned in 325 we jump in the if-block starting at
> 331. If sdap_id_op_done() finishes successful has_posix is still
> undefined but ret == EOK in line 346.
>
> 325     ret = sdap_posix_check_recv(subreq, &has_posix);
> 326     talloc_zfree(subreq);
> 327     if (ret != EOK) {
> 328         /* We can only finish the id_op on error as the connection
> 329          * is re-used by the user search
> 330          */
> 331         ret = sdap_id_op_done(state->op, ret, &dp_error);
> 332         if (dp_error == DP_ERR_OK && ret != EOK) {
> 333             /* retry */
> 334             ret = users_get_retry(req);
> 335             if (ret != EOK) {
> 336                 tevent_req_error(req, ret);
> 337             }
> 338             return;
> 339         }
> 340     }
> 341 
> 342     state->ctx->srv_opts->posix_checked = true;
> 343 
> 344     /* If the check ran to completion, we know for certain about the attributes
> 345      */
> 346     if (has_posix == false) {
> 347         state->sdap_ret = ERR_NO_POSIX;
> 348         tevent_req_done(req);
> 349         return;
> 350     }

You are right.
The variable ret is used for checking 3 functions sdap_posix_check_recv,
sdap_id_op_done, users_get_retry.

I decided to create another variable.

The other alternative is to initialize boolean variable has_posix to true,
becuase if there is any problem with connection we should still expect there
can be posix attributes.

LS

[Pavel Reichl]

On 01/27/2015 09:02 AM, Lukas Slebodnik wrote:
> On (26/01/15 20:14), Sumit Bose wrote:
> > I think there is still a chance that has_posix is uninitialised.
> > After ret != EOK is returned in 325 we jump in the if-block starting at
> > 331. If sdap_id_op_done() finishes successful has_posix is still
> > undefined but ret == EOK in line 346.
> >
> > 325     ret = sdap_posix_check_recv(subreq, &has_posix);
> > 326     talloc_zfree(subreq);
> > 327     if (ret != EOK) {
> > 328         /* We can only finish the id_op on error as the connection
> > 329          * is re-used by the user search
> > 330          */
> > 331         ret = sdap_id_op_done(state->op, ret, &dp_error);
> > 332         if (dp_error == DP_ERR_OK && ret != EOK) {
> > 333             /* retry */
> > 334             ret = users_get_retry(req);
> > 335             if (ret != EOK) {
> > 336                 tevent_req_error(req, ret);
> > 337             }
> > 338             return;
> > 339         }
> > 340     }
> > 341
> > 342     state->ctx->srv_opts->posix_checked = true;
> > 343
> > 344     /* If the check ran to completion, we know for certain about the attributes
> > 345      */
> > 346     if (has_posix == false) {
> > 347         state->sdap_ret = ERR_NO_POSIX;
> > 348         tevent_req_done(req);
> > 349         return;
> > 350     }
>
> You are right.
> The variable ret is used for checking 3 functions sdap_posix_check_recv,
> sdap_id_op_done, users_get_retry.
>
> I decided to create another variable.
>
> The other alternative is to initialize boolean variable has_posix to true,
> becuase if there is any problem with connection we should still expect there
> can be posix attributes.
>
> LS

Not nice, but I believe it address original problem and Sumit's comments 
comments as well.

So ACK by me.

ci: http://sssd-ci.duckdns.org/logs/job/7/73/summary.html
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Lukas Slebodnik]

On (18/02/15 18:32), Pavel Reichl wrote:
> On 01/27/2015 09:02 AM, Lukas Slebodnik wrote:
> > On (26/01/15 20:14), Sumit Bose wrote:
> > > I think there is still a chance that has_posix is uninitialised.
> > > After ret != EOK is returned in 325 we jump in the if-block starting at
> > > 331. If sdap_id_op_done() finishes successful has_posix is still
> > > undefined but ret == EOK in line 346.
> > >
> > > 325     ret = sdap_posix_check_recv(subreq, &has_posix);
> > > 326     talloc_zfree(subreq);
> > > 327     if (ret != EOK) {
> > > 328         /* We can only finish the id_op on error as the connection
> > > 329          * is re-used by the user search
> > > 330          */
> > > 331         ret = sdap_id_op_done(state->op, ret, &dp_error);
> > > 332         if (dp_error == DP_ERR_OK && ret != EOK) {
> > > 333             /* retry */
> > > 334             ret = users_get_retry(req);
> > > 335             if (ret != EOK) {
> > > 336                 tevent_req_error(req, ret);
> > > 337             }
> > > 338             return;
> > > 339         }
> > > 340     }
> > > 341
> > > 342     state->ctx->srv_opts->posix_checked = true;
> > > 343
> > > 344     /* If the check ran to completion, we know for certain about the attributes
> > > 345      */
> > > 346     if (has_posix == false) {
> > > 347         state->sdap_ret = ERR_NO_POSIX;
> > > 348         tevent_req_done(req);
> > > 349         return;
> > > 350     }
> >
> > You are right.
> > The variable ret is used for checking 3 functions sdap_posix_check_recv,
> > sdap_id_op_done, users_get_retry.
> >
> > I decided to create another variable.
> >
> > The other alternative is to initialize boolean variable has_posix to true,
> > becuase if there is any problem with connection we should still expect there
> > can be posix attributes.
> >
> > LS
>
> Not nice, but I believe it address original problem and Sumit's comments
> comments as well.
>
> So ACK by me.

If you prefer to initialize boolean variable has_posix to true
I can do it. (The patch will be simpler.)

LS

[Pavel Reichl]

On 02/18/2015 10:18 PM, Lukas Slebodnik wrote:
> On (18/02/15 18:32), Pavel Reichl wrote:
> > On 01/27/2015 09:02 AM, Lukas Slebodnik wrote:
> > > On (26/01/15 20:14), Sumit Bose wrote:
> > > > I think there is still a chance that has_posix is uninitialised.
> > > > After ret != EOK is returned in 325 we jump in the if-block starting at
> > > > 331. If sdap_id_op_done() finishes successful has_posix is still
> > > > undefined but ret == EOK in line 346.
> > > >
> > > > 325     ret = sdap_posix_check_recv(subreq, &has_posix);
> > > > 326     talloc_zfree(subreq);
> > > > 327     if (ret != EOK) {
> > > > 328         /* We can only finish the id_op on error as the connection
> > > > 329          * is re-used by the user search
> > > > 330          */
> > > > 331         ret = sdap_id_op_done(state->op, ret, &dp_error);
> > > > 332         if (dp_error == DP_ERR_OK && ret != EOK) {
> > > > 333             /* retry */
> > > > 334             ret = users_get_retry(req);
> > > > 335             if (ret != EOK) {
> > > > 336                 tevent_req_error(req, ret);
> > > > 337             }
> > > > 338             return;
> > > > 339         }
> > > > 340     }
> > > > 341
> > > > 342     state->ctx->srv_opts->posix_checked = true;
> > > > 343
> > > > 344     /* If the check ran to completion, we know for certain about the attributes
> > > > 345      */
> > > > 346     if (has_posix == false) {
> > > > 347         state->sdap_ret = ERR_NO_POSIX;
> > > > 348         tevent_req_done(req);
> > > > 349         return;
> > > > 350     }
> > >
> > > You are right.
> > > The variable ret is used for checking 3 functions sdap_posix_check_recv,
> > > sdap_id_op_done, users_get_retry.
> > >
> > > I decided to create another variable.
> > >
> > > The other alternative is to initialize boolean variable has_posix to true,
> > > becuase if there is any problem with connection we should still expect there
> > > can be posix attributes.
> > >
> > > LS
> >
> > Not nice, but I believe it address original problem and Sumit's comments
> > comments as well.
> >
> > So ACK by me.
>
> If you prefer to initialize boolean variable has_posix to true
> I can do it. (The patch will be simpler.)
>
> LS

Thanks for option but I don't like it any better. I think it's not a 
good programming practise to expect anything about output parameter 
(has_posix) of function in case it fails. I trust you it would work in 
this case, but generally....

I don't see any easy way how to provide a better fix for the bug, so I 
repeat - ACK to your patch.
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Lukas Slebodnik]

On (19/02/15 10:31), Pavel Reichl wrote:
> On 02/18/2015 10:18 PM, Lukas Slebodnik wrote:
> > On (18/02/15 18:32), Pavel Reichl wrote:
> > > On 01/27/2015 09:02 AM, Lukas Slebodnik wrote:
> > > > On (26/01/15 20:14), Sumit Bose wrote:
> > > > > I think there is still a chance that has_posix is uninitialised.
> > > > > After ret != EOK is returned in 325 we jump in the if-block starting at
> > > > > 331. If sdap_id_op_done() finishes successful has_posix is still
> > > > > undefined but ret == EOK in line 346.
> > > > >
> > > > > 325     ret = sdap_posix_check_recv(subreq, &has_posix);
> > > > > 326     talloc_zfree(subreq);
> > > > > 327     if (ret != EOK) {
> > > > > 328         /* We can only finish the id_op on error as the connection
> > > > > 329          * is re-used by the user search
> > > > > 330          */
> > > > > 331         ret = sdap_id_op_done(state->op, ret, &dp_error);
> > > > > 332         if (dp_error == DP_ERR_OK && ret != EOK) {
> > > > > 333             /* retry */
> > > > > 334             ret = users_get_retry(req);
> > > > > 335             if (ret != EOK) {
> > > > > 336                 tevent_req_error(req, ret);
> > > > > 337             }
> > > > > 338             return;
> > > > > 339         }
> > > > > 340     }
> > > > > 341
> > > > > 342     state->ctx->srv_opts->posix_checked = true;
> > > > > 343
> > > > > 344     /* If the check ran to completion, we know for certain about the attributes
> > > > > 345      */
> > > > > 346     if (has_posix == false) {
> > > > > 347         state->sdap_ret = ERR_NO_POSIX;
> > > > > 348         tevent_req_done(req);
> > > > > 349         return;
> > > > > 350     }
> > > >
> > > > You are right.
> > > > The variable ret is used for checking 3 functions sdap_posix_check_recv,
> > > > sdap_id_op_done, users_get_retry.
> > > >
> > > > I decided to create another variable.
> > > >
> > > > The other alternative is to initialize boolean variable has_posix to true,
> > > > becuase if there is any problem with connection we should still expect there
> > > > can be posix attributes.
> > > >
> > > > LS
> > >
> > > Not nice, but I believe it address original problem and Sumit's comments
> > > comments as well.
> > >
> > > So ACK by me.
> >
> > If you prefer to initialize boolean variable has_posix to true
> > I can do it. (The patch will be simpler.)
> >
> > LS
>
> Thanks for option but I don't like it any better. I think it's not a good
> programming practise to expect anything about output parameter (has_posix) of
> function in case it fails. I trust you it would work in this case, but
> generally....

I had the same feeling.
> I don't see any easy way how to provide a better fix for the bug, so I repeat
> ACK to your patch.

Thank you for review.

LS

[Jakub Hrozek]

On Thu, Feb 19, 2015 at 10:31:52AM +0100, Pavel Reichl wrote:
> I don't see any easy way how to provide a better fix for the bug, so I
> repeat - ACK to your patch.

sorry for the delay in pushing, the patch is in master now:
    35808a6c8cea7baef659192dbb981872f95337ea

please let me know if you'd like to have this patch pushed to sssd-1-12
also.