From 85dbf449c02633e764df52d47a12f442e46528d7 Mon Sep 17 00:00:00 2001 From: Marko Lindqvist Date: Sun, 11 Sep 2022 09:28:09 +0300 Subject: [PATCH 50/50] Fix memory leaks from req_to_fstring() usage See osdn #45544 Signed-off-by: Marko Lindqvist --- common/actions.c | 15 ++++++++++---- common/requirements.c | 42 +++++++++++++++++++++++++++++---------- common/requirements.h | 8 ++++++-- server/rssanity.c | 20 ++++++++++++++----- tools/ruleutil/rulesave.c | 5 ++++- 5 files changed, 68 insertions(+), 22 deletions(-) diff --git a/common/actions.c b/common/actions.c index 84b2138069..664942e119 100644 --- a/common/actions.c +++ b/common/actions.c @@ -2667,8 +2667,8 @@ enabler_tile_tgt_local_diplrel_implies_claimed( struct requirement *claimed_req; struct requirement tile_is_claimed; struct requirement tile_is_unclaimed; - struct action *paction = action_by_number(enabler->action); + struct astring astr; if (action_get_target_kind(paction) != ATK_TILE) { /* Not tile targeted */ @@ -2712,7 +2712,9 @@ enabler_tile_tgt_local_diplrel_implies_claimed( * so this is implicit.) */ N_("Requirement {%s} of action \"%s\" implies a claimed " "tile. No diplomatic relation to Nature."), - req_to_fstring(local_diplrel), action_rule_name(paction)); + req_to_fstring(local_diplrel, &astr), action_rule_name(paction)); + + astr_free(&astr); /* The solution is to add the requirement that the tile is claimed */ out->suggested_solutions[0].req = tile_is_claimed; @@ -2736,8 +2738,9 @@ enabler_first_self_contradiction(const struct action_enabler *enabler) struct requirement *local_diplrel; struct requirement *unclaimed_req; struct requirement tile_is_claimed; - struct action *paction = action_by_number(enabler->action); + struct astring astr1; + struct astring astr2; if (action_get_target_kind(paction) != ATK_TILE) { /* Not tile targeted */ @@ -2774,7 +2777,11 @@ enabler_first_self_contradiction(const struct action_enabler *enabler) N_("In enabler for \"%s\": No diplomatic relation to Nature." " Requirements {%s} and {%s} contradict each other."), action_rule_name(paction), - req_to_fstring(local_diplrel), req_to_fstring(unclaimed_req)); + req_to_fstring(local_diplrel, &astr1), + req_to_fstring(unclaimed_req, &astr2)); + + astr_free(&astr1); + astr_free(&astr2); /* The first suggestion is to remove the diplrel */ out->suggested_solutions[0].req = *local_diplrel; diff --git a/common/requirements.c b/common/requirements.c index 44775b74ca..3d2c3e2d84 100644 --- a/common/requirements.c +++ b/common/requirements.c @@ -726,19 +726,24 @@ const struct req_context *req_context_empty(void) /**********************************************************************//** Returns the given requirement as a formatted string ready for printing. Does not care about the 'quiet' property. + + astring does not need to be initialized before the call, + but caller needs to call astr_free() for it once the returned + string is no longer needed. **************************************************************************/ -const char *req_to_fstring(const struct requirement *req) +const char *req_to_fstring(const struct requirement *req, + struct astring *astr) { - struct astring printable_req = ASTRING_INIT; + astr_init(astr); - astr_set(&printable_req, "%s%s %s %s%s", + astr_set(astr, "%s%s %s %s%s", req->survives ? "surviving " : "", req_range_name(req->range), universal_type_rule_name(&req->source), req->present ? "" : "!", universal_rule_name(&req->source)); - return astr_str(&printable_req); + return astr_str(astr); } /**********************************************************************//** @@ -3696,6 +3701,7 @@ const char *req_vec_change_translation(const struct req_vec_change *change, { const char *req_vec_description; static char buf[MAX_LEN_NAME * 3]; + struct astring astr; fc_assert_ret_val(change, NULL); fc_assert_ret_val(req_vec_change_operation_is_valid(change->operation), @@ -3723,8 +3729,9 @@ const char *req_vec_change_translation(const struct req_vec_change *change, * like "actor_reqs" */ _("%s %s from %s"), req_vec_change_operation_name(change->operation), - req_to_fstring(&change->req), + req_to_fstring(&change->req, &astr), req_vec_description); + astr_free(&astr); break; case RVCO_APPEND: fc_snprintf(buf, sizeof(buf), @@ -3736,8 +3743,9 @@ const char *req_vec_change_translation(const struct req_vec_change *change, * like "actor_reqs" */ _("%s %s to %s"), req_vec_change_operation_name(change->operation), - req_to_fstring(&change->req), + req_to_fstring(&change->req, &astr), req_vec_description); + astr_free(&astr); break; case RVCO_NOOP: fc_snprintf(buf, sizeof(buf), @@ -3904,10 +3912,15 @@ req_vec_get_first_contradiction(const struct requirement_vector *vec, if (are_requirements_contradictions(preq, nreq)) { struct req_vec_problem *problem; + struct astring astr; + struct astring nastr; problem = req_vec_problem_new(2, N_("Requirements {%s} and {%s} contradict each other."), - req_to_fstring(preq), req_to_fstring(nreq)); + req_to_fstring(preq, &astr), req_to_fstring(nreq, &nastr)); + + astr_free(&astr); + astr_free(&nastr); /* The solution is to remove one of the contradictions. */ problem->suggested_solutions[0].operation = RVCO_REMOVE; @@ -3983,6 +3996,7 @@ req_vec_get_first_missing_univ(const struct requirement_vector *vec, if (universal_never_there(&preq->source)) { struct req_vec_problem *problem; + struct astring astr; if (preq->present) { /* The requirement vector can never be fulfilled. Removing the @@ -3995,13 +4009,16 @@ req_vec_get_first_missing_univ(const struct requirement_vector *vec, req_vec_problem_new(0, N_("Requirement {%s} requires %s but it will never be" " there."), - req_to_fstring(preq), universal_rule_name(&preq->source)); + req_to_fstring(preq, &astr), universal_rule_name(&preq->source)); + astr_free(&astr); continue; } problem = req_vec_problem_new(1, N_("Requirement {%s} mentions %s but it will never be there."), - req_to_fstring(preq), universal_rule_name(&preq->source)); + req_to_fstring(preq, &astr), universal_rule_name(&preq->source)); + + astr_free(&astr); /* The solution is to remove the reference to the missing * universal. */ @@ -4056,10 +4073,15 @@ req_vec_get_first_redundant_req(const struct requirement_vector *vec, if (are_requirements_equal(preq, nreq)) { struct req_vec_problem *problem; + struct astring astr; + struct astring nastr; problem = req_vec_problem_new(2, N_("Requirements {%s} and {%s} are the same."), - req_to_fstring(preq), req_to_fstring(nreq)); + req_to_fstring(preq, &astr), req_to_fstring(nreq, &nastr)); + + astr_free(&astr); + astr_free(&nastr); /* The solution is to remove one of the redundant requirements. */ problem->suggested_solutions[0].operation = RVCO_REMOVE; diff --git a/common/requirements.h b/common/requirements.h index 3d3092beb4..84e2f93aa7 100644 --- a/common/requirements.h +++ b/common/requirements.h @@ -18,6 +18,9 @@ extern "C" { #endif /* __cplusplus */ +/* utility */ +#include "astring.h" + /* common */ #include "fc_types.h" @@ -111,7 +114,8 @@ const struct req_context *req_context_empty(void); struct requirement req_from_str(const char *type, const char *range, bool survives, bool present, bool quiet, const char *value); -const char *req_to_fstring(const struct requirement *req); +const char *req_to_fstring(const struct requirement *req, + struct astring *astr); void req_get_values(const struct requirement *req, int *type, int *range, bool *survives, bool *present, bool *quiet, @@ -121,7 +125,7 @@ struct requirement req_from_values(int type, int range, int value); bool are_requirements_equal(const struct requirement *req1, - const struct requirement *req2); + const struct requirement *req2); bool are_requirements_contradictions(const struct requirement *req1, const struct requirement *req2); diff --git a/server/rssanity.c b/server/rssanity.c index 0d444865a9..99e94f1228 100644 --- a/server/rssanity.c +++ b/server/rssanity.c @@ -532,6 +532,7 @@ static bool effect_list_sanity_cb(struct effect *peffect, void *data) int one_tile = -1; /* TODO: Determine correct value from effect. * -1 disables checking */ els_data *els = (els_data *)data; + struct astring astr; /* TODO: Refactor this to be more reusable when we check * for more than one base effect. */ @@ -553,8 +554,9 @@ static bool effect_list_sanity_cb(struct effect *peffect, void *data) "The effect Action_Success_Target_Move_Cost has the" " requirement {%s} but the action %s isn't" " (single) unit targeted.", - req_to_fstring(preq), + req_to_fstring(preq, &astr), universal_rule_name(&preq->source)); + astr_free(&astr); return FALSE; } } @@ -568,8 +570,9 @@ static bool effect_list_sanity_cb(struct effect *peffect, void *data) "The effect Action_Success_Actor_Move_Cost has the" " requirement {%s} but the action %s isn't" " performed by a unit.", - req_to_fstring(preq), + req_to_fstring(preq, &astr), universal_rule_name(&preq->source)); + astr_free(&astr); return FALSE; } } @@ -584,8 +587,9 @@ static bool effect_list_sanity_cb(struct effect *peffect, void *data) "The effect Action_Odds_Pct has the" " requirement {%s} but the action %s doesn't" " roll the dice to see if it fails.", - req_to_fstring(preq), + req_to_fstring(preq, &astr), universal_rule_name(&preq->source)); + astr_free(&astr); return FALSE; } } @@ -894,6 +898,8 @@ bool sanity_check_ruleset_data(struct rscompat_info *compat) advance_rule_name(padvance)); ok = FALSE; } else if (!is_req_unchanging(preq)) { + struct astring astr; + /* Only support unchanging requirements until the reachability code * can handle it and the tech tree can display changing * requirements. */ @@ -903,7 +909,8 @@ bool sanity_check_ruleset_data(struct rscompat_info *compat) " the game. Changing requirements aren't supported" " yet.", advance_rule_name(padvance), - req_to_fstring(preq)); + req_to_fstring(preq, &astr)); + astr_free(&astr); ok = FALSE; } } requirement_vector_iterate_end; @@ -1285,6 +1292,8 @@ bool sanity_check_ruleset_data(struct rscompat_info *compat) requirement_vector_iterate(&(enabler->target_reqs), preq) { if (preq->source.kind == VUT_DIPLREL && preq->range == REQ_RANGE_LOCAL) { + struct astring astr; + /* A Local DiplRel requirement can be expressed as a requirement * in actor_reqs. Demand that it is there. This avoids breaking * code that reasons about actions. */ @@ -1294,7 +1303,8 @@ bool sanity_check_ruleset_data(struct rscompat_info *compat) "section \"Requirement vector rules\" in " "doc/README.actions", action_id_rule_name(act), - req_to_fstring(preq)); + req_to_fstring(preq, &astr)); + astr_free(&astr); ok = FALSE; } } requirement_vector_iterate_end; diff --git a/tools/ruleutil/rulesave.c b/tools/ruleutil/rulesave.c index 0dfa2df641..34905be27e 100644 --- a/tools/ruleutil/rulesave.c +++ b/tools/ruleutil/rulesave.c @@ -529,8 +529,11 @@ static bool save_action_auto_uflag_block(struct section_file *sfile, protecor_flag[i++] = req->source.value.unitflag; } else if (unexpected_req(req)) { + struct astring astr; + log_error("Can't handle action auto performer requirement %s", - req_to_fstring(req)); + req_to_fstring(req, &astr)); + astr_free(&astr); return FALSE; } -- 2.35.1