From b665c4cd062fafad4a57254af1376381944cf3ea Mon Sep 17 00:00:00 2001 From: Haru Date: Tue, 30 Apr 2024 20:48:32 +0200 Subject: [PATCH] Fix some issues in getitemgroupitems - Memory leak in the deduplication function - Some unnecessary data copies in the search and deduplication function - Accidental overwrite of the original group's data in the deduplication function Signed-off-by: Haru --- src/map/itemdb.c | 24 ++++++------- src/map/itemdb.h | 2 +- src/map/script.c | 90 ++++++++++++++---------------------------------- 3 files changed, 36 insertions(+), 80 deletions(-) diff --git a/src/map/itemdb.c b/src/map/itemdb.c index 58569d053c1..364030d7e3e 100644 --- a/src/map/itemdb.c +++ b/src/map/itemdb.c @@ -348,23 +348,19 @@ static bool itemdb_in_group(struct item_group *group, int nameid) } /** - * Search for a group of item - * @param group return information about group + * Search for an item group. + * * @param nameid item group id to search - * @return bool true if found, false otherwise + * @return A pointer to the group + * @retval NULL if the group was not found */ -static bool itemdb_search_group(struct item_group *group, int nameid) +static const struct item_group *itemdb_search_group(int nameid) { - for (int i = 0; i < itemdb->group_count; i++) { - struct item_group g = itemdb->groups[i]; - - if (g.id == nameid) { - *group = g; - return true; - } - } - - return false; + int i; + ARR_FIND(0, itemdb->group_count, i, itemdb->groups[i].id == nameid); + if (i != itemdb->group_count) + return &itemdb->groups[i]; + return NULL; } /// Searches for the item_data. diff --git a/src/map/itemdb.h b/src/map/itemdb.h index 573d13fdb8c..725886d0b21 100644 --- a/src/map/itemdb.h +++ b/src/map/itemdb.h @@ -708,7 +708,7 @@ struct itemdb_interface { struct itemdb_option* (*option_exists) (int idx); struct item_reform* (*reform_exists) (int idx); bool (*in_group) (struct item_group *group, int nameid); - bool (*search_group) (struct item_group *group, int nameid); + const struct item_group *(*search_group) (int nameid); int (*group_item) (struct item_group *group); int (*chain_item) (unsigned short chain_id, int *rate); void (*package_item) (struct map_session_data *sd, struct item_package *package); diff --git a/src/map/script.c b/src/map/script.c index 0186773d6d2..9f04bf934fb 100644 --- a/src/map/script.c +++ b/src/map/script.c @@ -9133,46 +9133,6 @@ static BUILDIN(grouprandomitem) return true; } -/*========================================== - * utils for item group - *------------------------------------------*/ -static bool utils_itemgroup_remove_duplicate(struct item_group *group) -{ - int n = group->qty; - int *arr = group->nameid; - int i, j, k; - - // Remove duplicates - for (i = 0; i < n; i++) { - for (j = i + 1; j < n; j++) { - if (arr[i] == arr[j]) { - // Shift elements to the left - for (k = j; k < n - 1; k++) { - arr[k] = arr[k + 1]; - } - n--; - j--; - } - } - } - - // Create a new array without duplicates - int *newArr = malloc(n * sizeof(int)); - if (newArr == NULL) { - // Handle memory allocation error - return false; - } - - for (i = 0; i < n; i++) { - newArr[i] = arr[i]; - } - - group->qty = n; - group->nameid = newArr; - - return true; -} - /*========================================== * getitemgroupitems , ; *------------------------------------------*/ @@ -9188,27 +9148,6 @@ static BUILDIN(getitemgroupitems) return false;// not a variable } - int nameid = script_getnum(st, 3); - struct item_data *item = itemdb->exists(nameid); - if (item == NULL) { - ShowError("buildin_getitemgroupitems: invalid item %d\n", nameid); - script_pushint(st, 0); - return false; - } - - struct item_group group; - if (itemdb->search_group(&group, item->nameid) == false) { - ShowWarning("buildin_getitemgroupitems: item group not found\n"); - script_pushint(st, 0); - return false; - } - - if (!utils_itemgroup_remove_duplicate(&group)) { - ShowError("buildin_getitemgroupitems: failed to remove duplicates\n"); - script_pushint(st, 0); - return false; - } - int32 idata = reference_getindex(data); int32 dataid = reference_getid(data); const char *data_name = reference_getname(data); @@ -9228,12 +9167,33 @@ static BUILDIN(getitemgroupitems) st->state = END; return false; } - + + int nameid = script_getnum(st, 3); + struct item_data *item = itemdb->exists(nameid); + if (item == NULL) { + ShowError("buildin_getitemgroupitems: invalid item %d\n", nameid); + script_pushint(st, 0); + return false; + } + + const struct item_group *group = itemdb->search_group(item->nameid); + if (group == NULL) { + ShowWarning("buildin_getitemgroupitems: item group not found\n"); + script_pushint(st, 0); + return false; + } + int count = 0; - for (int i = 0; i < group.qty; i++) { - int id = group.nameid[i]; + for (int i = 0; i < group->qty; i++) { + int id = group->nameid[i]; + int j = 0; + ARR_FIND(0, i, j, group->nameid[i] == group->nameid[j]); + if (i != j) { + // Already encountered - skip duplicates + continue; + } const void *v = (const void *)h64BPTRSIZE(id); - script->set_reg(st, NULL, reference_uid(dataid, idata + i), data_name, v, reference_getref(data)); + script->set_reg(st, NULL, reference_uid(dataid, idata + count), data_name, v, reference_getref(data)); count++; }