From: Arran Cudbard-Bell Date: Sun, 1 Jun 2025 18:12:24 +0000 (-0600) Subject: Add sanity check in unlang_module_yield X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c71ea2a06e676e4e007c8aa3b731073e40a1abbb;p=thirdparty%2Ffreeradius-server.git Add sanity check in unlang_module_yield Why? Because if the yield is done _AFTER_ a function has been pushed onto the stack, we get undefined reuslts --- diff --git a/src/lib/unlang/module.c b/src/lib/unlang/module.c index 7586403a31a..f6d84031d0d 100644 --- a/src/lib/unlang/module.c +++ b/src/lib/unlang/module.c @@ -429,6 +429,7 @@ unlang_action_t unlang_module_yield_to_retry(request_t *request, module_method_t * @param[in] rctx to pass to the callbacks. * @return * - UNLANG_ACTION_YIELD. + * - UNLANG_ACTION_FAIL if this is not a module frame. */ unlang_action_t unlang_module_yield(request_t *request, module_method_t resume, unlang_module_signal_t signal, fr_signal_t sigmask, void *rctx) @@ -439,6 +440,11 @@ unlang_action_t unlang_module_yield(request_t *request, REQUEST_VERIFY(request); /* Check the yielded request is sane */ + if (frame->instruction->type != UNLANG_TYPE_MODULE) { + fr_assert_msg(0, "unlang_module_yield called on a non-module frame"); + return UNLANG_ACTION_FAIL; + } + state->rctx = rctx; state->resume = resume; diff --git a/src/modules/rlm_ldap/rlm_ldap.c b/src/modules/rlm_ldap/rlm_ldap.c index 0dcebf5be65..aad39441d9c 100644 --- a/src/modules/rlm_ldap/rlm_ldap.c +++ b/src/modules/rlm_ldap/rlm_ldap.c @@ -1606,7 +1606,7 @@ static unlang_action_t CC_HINT(nonnull) mod_authenticate(unlang_result_t *p_resu } #define REPEAT_MOD_AUTHORIZE_RESUME \ - if (unlang_module_set_resume(request, mod_authorize_resume) < 0) do { \ + if (unlang_module_yield(request, mod_authorize_resume, NULL, 0, autz_ctx) == UNLANG_ACTION_FAIL) do { \ p_result->rcode = RLM_MODULE_FAIL; \ goto finish; \ } while (0) diff --git a/src/modules/rlm_mschap/rlm_mschap.c b/src/modules/rlm_mschap/rlm_mschap.c index 519e6b9562e..ac6ae5af397 100644 --- a/src/modules/rlm_mschap/rlm_mschap.c +++ b/src/modules/rlm_mschap/rlm_mschap.c @@ -2305,7 +2305,7 @@ static unlang_action_t CC_HINT(nonnull) mod_authenticate(unlang_result_t *p_resu #ifdef WITH_TLS if (mschap_new_pass_decrypt(request, auth_ctx) < 0) RETURN_UNLANG_FAIL; - if (unlang_module_yield(request, mod_authenticate_resume, NULL, 0, auth_ctx) != UNLANG_ACTION_YIELD) { + if (unlang_module_yield(request, mod_authenticate_resume, NULL, 0, auth_ctx) == UNLANG_ACTION_FAIL) { RETURN_UNLANG_FAIL; } @@ -2327,7 +2327,7 @@ static unlang_action_t CC_HINT(nonnull) mod_authenticate(unlang_result_t *p_resu /* * Run the resumption function where we're done with: */ - if (unlang_module_yield(request, mod_authenticate_resume, NULL, 0, auth_ctx) != UNLANG_ACTION_YIELD) { + if (unlang_module_yield(request, mod_authenticate_resume, NULL, 0, auth_ctx) == UNLANG_ACTION_FAIL) { RETURN_UNLANG_FAIL; }; diff --git a/src/modules/rlm_sql/rlm_sql.c b/src/modules/rlm_sql/rlm_sql.c index aa9ed594cfa..72f3847b747 100644 --- a/src/modules/rlm_sql/rlm_sql.c +++ b/src/modules/rlm_sql/rlm_sql.c @@ -1292,7 +1292,7 @@ static unlang_action_t CC_HINT(nonnull) mod_autz_group_resume(unlang_result_t * switch(autz_ctx->status) { case SQL_AUTZ_GROUP_MEMB: - if (unlang_module_yield(request, mod_autz_group_resume, NULL, 0, mctx->rctx) != UNLANG_ACTION_YIELD) RETURN_UNLANG_FAIL; + if (unlang_module_yield(request, mod_autz_group_resume, NULL, 0, mctx->rctx) == UNLANG_ACTION_FAIL) RETURN_UNLANG_FAIL; MEM(autz_ctx->group_ctx = talloc(autz_ctx, sql_group_ctx_t)); *autz_ctx->group_ctx = (sql_group_ctx_t) { .inst = inst, @@ -1341,7 +1341,7 @@ static unlang_action_t CC_HINT(nonnull) mod_autz_group_resume(unlang_result_t * } if (call_env->group_check_query) { - if (unlang_module_yield(request, mod_autz_group_resume, NULL, 0, mctx->rctx) != UNLANG_ACTION_YIELD) RETURN_UNLANG_FAIL; + if (unlang_module_yield(request, mod_autz_group_resume, NULL, 0, mctx->rctx) == UNLANG_ACTION_FAIL) RETURN_UNLANG_FAIL; if (unlang_tmpl_push(autz_ctx, &autz_ctx->query, request, call_env->group_check_query, NULL) < 0) RETURN_UNLANG_FAIL; return UNLANG_ACTION_PUSHED_CHILD; @@ -1361,7 +1361,7 @@ static unlang_action_t CC_HINT(nonnull) mod_autz_group_resume(unlang_result_t * .query = query, }; - if (unlang_module_yield(request, mod_autz_group_resume, NULL, 0, mctx->rctx) != UNLANG_ACTION_YIELD) RETURN_UNLANG_FAIL; + if (unlang_module_yield(request, mod_autz_group_resume, NULL, 0, mctx->rctx) == UNLANG_ACTION_FAIL) RETURN_UNLANG_FAIL; if (sql_get_map_list(request, map_ctx, autz_ctx->trunk) == UNLANG_ACTION_PUSHED_CHILD) { autz_ctx->status = autz_ctx->status & SQL_AUTZ_STAGE_GROUP ? SQL_AUTZ_GROUP_CHECK_RESUME : SQL_AUTZ_PROFILE_CHECK_RESUME; return UNLANG_ACTION_PUSHED_CHILD; @@ -1395,7 +1395,7 @@ static unlang_action_t CC_HINT(nonnull) mod_autz_group_resume(unlang_result_t * if (call_env->group_reply_query) { group_reply_push: - if (unlang_module_yield(request, mod_autz_group_resume, NULL, 0, mctx->rctx) != UNLANG_ACTION_YIELD) RETURN_UNLANG_FAIL; + if (unlang_module_yield(request, mod_autz_group_resume, NULL, 0, mctx->rctx) == UNLANG_ACTION_FAIL) RETURN_UNLANG_FAIL; if (unlang_tmpl_push(autz_ctx, &autz_ctx->query, request, call_env->group_reply_query, NULL) < 0) RETURN_UNLANG_FAIL; autz_ctx->status = autz_ctx->status & SQL_AUTZ_STAGE_GROUP ? SQL_AUTZ_GROUP_REPLY : SQL_AUTZ_PROFILE_REPLY; @@ -1417,7 +1417,7 @@ static unlang_action_t CC_HINT(nonnull) mod_autz_group_resume(unlang_result_t * .expand_rhs = true, }; - if (unlang_module_yield(request, mod_autz_group_resume, NULL, 0, mctx->rctx) != UNLANG_ACTION_YIELD) RETURN_UNLANG_FAIL; + if (unlang_module_yield(request, mod_autz_group_resume, NULL, 0, mctx->rctx) == UNLANG_ACTION_FAIL) RETURN_UNLANG_FAIL; if (sql_get_map_list(request, map_ctx, autz_ctx->trunk) == UNLANG_ACTION_PUSHED_CHILD) { autz_ctx->status = autz_ctx->status & SQL_AUTZ_STAGE_GROUP ? SQL_AUTZ_GROUP_REPLY_RESUME : SQL_AUTZ_PROFILE_REPLY_RESUME; return UNLANG_ACTION_PUSHED_CHILD; @@ -1531,7 +1531,7 @@ static unlang_action_t CC_HINT(nonnull) mod_authorize_resume(unlang_result_t *p_ .query = query, }; - if (unlang_module_yield(request, mod_authorize_resume, NULL, 0, autz_ctx) != UNLANG_ACTION_YIELD) RETURN_UNLANG_FAIL; + if (unlang_module_yield(request, mod_authorize_resume, NULL, 0, autz_ctx) == UNLANG_ACTION_FAIL) RETURN_UNLANG_FAIL; if (sql_get_map_list(request, map_ctx, autz_ctx->trunk) == UNLANG_ACTION_PUSHED_CHILD){ autz_ctx->status = SQL_AUTZ_CHECK_RESUME; return UNLANG_ACTION_PUSHED_CHILD; @@ -1558,7 +1558,7 @@ static unlang_action_t CC_HINT(nonnull) mod_authorize_resume(unlang_result_t *p_ if (!call_env->reply_query) goto skip_reply; - if (unlang_module_yield(request, mod_authorize_resume, NULL, 0, autz_ctx) != UNLANG_ACTION_YIELD) RETURN_UNLANG_FAIL; + if (unlang_module_yield(request, mod_authorize_resume, NULL, 0, autz_ctx) == UNLANG_ACTION_FAIL) RETURN_UNLANG_FAIL; if (unlang_tmpl_push(autz_ctx, &autz_ctx->query, request, call_env->reply_query, NULL) < 0) RETURN_UNLANG_FAIL; autz_ctx->status = SQL_AUTZ_REPLY; return UNLANG_ACTION_PUSHED_CHILD; @@ -1573,7 +1573,7 @@ static unlang_action_t CC_HINT(nonnull) mod_authorize_resume(unlang_result_t *p_ .expand_rhs = true, }; - if (unlang_module_yield(request, mod_authorize_resume, NULL, 0, autz_ctx) != UNLANG_ACTION_YIELD) RETURN_UNLANG_FAIL; + if (unlang_module_yield(request, mod_authorize_resume, NULL, 0, autz_ctx) == UNLANG_ACTION_FAIL) RETURN_UNLANG_FAIL; if (sql_get_map_list(request, map_ctx, autz_ctx->trunk) == UNLANG_ACTION_PUSHED_CHILD){ autz_ctx->status = SQL_AUTZ_REPLY_RESUME; return UNLANG_ACTION_PUSHED_CHILD; @@ -1620,7 +1620,7 @@ static unlang_action_t CC_HINT(nonnull) mod_authorize_resume(unlang_result_t *p_ break; } - if (unlang_module_yield(request, mod_autz_group_resume, NULL, 0, autz_ctx) != UNLANG_ACTION_YIELD) RETURN_UNLANG_FAIL; + if (unlang_module_yield(request, mod_autz_group_resume, NULL, 0, autz_ctx) == UNLANG_ACTION_FAIL) RETURN_UNLANG_FAIL; if (unlang_tmpl_push(autz_ctx, &autz_ctx->query, request, call_env->membership_query, NULL) < 0) RETURN_UNLANG_FAIL; autz_ctx->status = SQL_AUTZ_GROUP_MEMB; @@ -1692,7 +1692,7 @@ static unlang_action_t CC_HINT(nonnull) mod_authorize(unlang_result_t *p_result, if (unlang_module_yield(request, (call_env->check_query || call_env->reply_query) ? mod_authorize_resume : mod_autz_group_resume, - NULL, 0, autz_ctx) != UNLANG_ACTION_YIELD) { + NULL, 0, autz_ctx) == UNLANG_ACTION_FAIL) { error: talloc_free(autz_ctx); RETURN_UNLANG_FAIL; @@ -1814,7 +1814,7 @@ next: talloc_free(query_ctx); redundant_ctx->query_no++; if (redundant_ctx->query_no >= talloc_array_length(call_env->query)) RETURN_UNLANG_NOOP; - if (unlang_module_yield(request, mod_sql_redundant_resume, NULL, 0, redundant_ctx) < 0) RETURN_UNLANG_FAIL; + if (unlang_module_yield(request, mod_sql_redundant_resume, NULL, 0, redundant_ctx) == UNLANG_ACTION_FAIL) RETURN_UNLANG_FAIL; if (unlang_tmpl_push(redundant_ctx, &redundant_ctx->query, request, call_env->query[redundant_ctx->query_no], NULL) < 0) RETURN_UNLANG_FAIL; RDEBUG2("Trying next query..."); diff --git a/src/modules/rlm_sqlcounter/rlm_sqlcounter.c b/src/modules/rlm_sqlcounter/rlm_sqlcounter.c index 5a3c481cf94..42a588640a9 100644 --- a/src/modules/rlm_sqlcounter/rlm_sqlcounter.c +++ b/src/modules/rlm_sqlcounter/rlm_sqlcounter.c @@ -430,7 +430,7 @@ static unlang_action_t CC_HINT(nonnull) mod_authorize(unlang_result_t *p_result, .limit = limit }; - if (unlang_module_yield(request, mod_authorize_resume, NULL, 0, rctx) != UNLANG_ACTION_YIELD) { + if (unlang_module_yield(request, mod_authorize_resume, NULL, 0, rctx) == UNLANG_ACTION_FAIL) { error: talloc_free(rctx); RETURN_UNLANG_FAIL; diff --git a/src/modules/rlm_sqlippool/rlm_sqlippool.c b/src/modules/rlm_sqlippool/rlm_sqlippool.c index e87e6f555f0..93a821f3a22 100644 --- a/src/modules/rlm_sqlippool/rlm_sqlippool.c +++ b/src/modules/rlm_sqlippool/rlm_sqlippool.c @@ -225,7 +225,7 @@ static int sqlippool_alloc_ctx_free(ippool_alloc_ctx_t *to_free) return 0; } -#define REPEAT_MOD_ALLOC_RESUME if (unlang_module_yield(request, mod_alloc_resume, NULL, 0, mctx->rctx) < 0) RETURN_UNLANG_FAIL +#define REPEAT_MOD_ALLOC_RESUME if (unlang_module_yield(request, mod_alloc_resume, NULL, 0, mctx->rctx) == UNLANG_ACTION_FAIL) RETURN_UNLANG_FAIL #define SUBMIT_QUERY(_query_str, _new_status, _type, _function) do { \ alloc_ctx->status = _new_status; \ REPEAT_MOD_ALLOC_RESUME; \