From 57e4b56fac028f83b7a467001ea982b47ab44580 Mon Sep 17 00:00:00 2001 From: Milan Crha Date: Mon, 14 Oct 2019 21:23:20 +0200 Subject: Let icalattach_new_from_data() use the 'free_fn' argument again This had been disabled with commit 7a2f318bacae5521848963d96a3a644bc6be1d01 (in time of 1.0.1) to fix inline attachments crashing. That commit could eventually cause memory leaks and higher memory usage. This change adds necessary strdup() calls on places where required only and makes use of the 'free_fn' argument again. It also fixes libical-glib's i_cal_attach_new_from_bytes(), which relies on this 'free_fn'. --- ReleaseNotes.txt | 1 + src/libical-glib/api/i-cal-attach.xml | 22 +++++++++++++--- src/libical/icalattach.c | 17 +++---------- src/libical/icalattach.h | 8 +++--- src/libical/icalderivedvalue.c.in | 8 +++++- src/libical/icalvalue.c | 8 +++++- src/test/libical-glib/attach.py | 2 +- src/test/regression.c | 48 +++++++++++++++++++++++++++++++++-- 8 files changed, 87 insertions(+), 27 deletions(-) diff --git a/ReleaseNotes.txt b/ReleaseNotes.txt index f580fa4b..0b5c5c8d 100644 --- a/ReleaseNotes.txt +++ b/ReleaseNotes.txt @@ -4,6 +4,7 @@ Release Highlights Version 3.0.7 (UNRELEASED): --------------------------- * libical-glib: Fix ICalAttach handling of the icalattach native structure + * Let icalattach_new_from_data() use the 'free_fn' argument again Version 3.0.6 (14 Sep 2019): ---------------------------- diff --git a/src/libical-glib/api/i-cal-attach.xml b/src/libical-glib/api/i-cal-attach.xml index e4fcf0d2..c77c0102 100644 --- a/src/libical-glib/api/i-cal-attach.xml +++ b/src/libical-glib/api/i-cal-attach.xml @@ -19,27 +19,41 @@ Create a new #ICalAttach from the url - + +static void icalglib_free_attach_data(gpointer data, gpointer user_data) +{ + _unused(user_data); + g_free (data); +} + Create a new #ICalAttach from the data. + g_return_val_if_fail (data != NULL, NULL); + + if (!free_fn) { + data = g_strdup(data); + free_fn = icalglib_free_attach_data; + } + + return i_cal_attach_new_full (icalattach_new_from_data (data, (icalattach_free_fn_t) (free_fn), free_fn_data), NULL); -static void unref_g_bytes(unsigned char *data, void *user_data) +static void unref_g_bytes(char *data, void *user_data) { GBytes *bytes = user_data; g_return_if_fail (data != NULL); g_return_if_fail (bytes != NULL); - g_bytes_unref (bytes); + g_bytes_unref(bytes); } - Create a new #ICalAttach from the data in bytes. Takes a reference of @bytes, increase the reference before calling this function if you with to use it afterward. + Create a new #ICalAttach from the data in bytes. Takes a reference of @bytes, increase the reference before calling this function if you with to use it afterward. The stored bytes should be already encoded with used encoding (like base64). g_return_val_if_fail (bytes != NULL, NULL); return i_cal_attach_new_full (icalattach_new_from_data (g_bytes_get_data (bytes, NULL), unref_g_bytes, bytes), NULL); diff --git a/src/libical/icalattach.c b/src/libical/icalattach.c index e5c3a097..d66a67f7 100644 --- a/src/libical/icalattach.c +++ b/src/libical/icalattach.c @@ -55,7 +55,6 @@ icalattach *icalattach_new_from_data(const char *data, icalattach_free_fn_t free void *free_fn_data) { icalattach *attach; - char *data_copy; icalerror_check_arg_rz((data != NULL), "data"); @@ -64,15 +63,9 @@ icalattach *icalattach_new_from_data(const char *data, icalattach_free_fn_t free return NULL; } - if ((data_copy = strdup(data)) == NULL) { - free(attach); - errno = ENOMEM; - return NULL; - } - attach->refcount = 1; attach->is_url = 0; - attach->u.data.data = data_copy; + attach->u.data.data = (char *) data; attach->u.data.free_fn = free_fn; attach->u.data.free_fn_data = free_fn_data; @@ -99,12 +92,8 @@ void icalattach_unref(icalattach *attach) if (attach->is_url) { free(attach->u.url.url); - } else { - free(attach->u.data.data); -/* unused for now - if (attach->u.data.free_fn) - (* attach->u.data.free_fn) (attach->u.data.data, attach->u.data.free_fn_data); -*/ + } else if (attach->u.data.free_fn) { + (* attach->u.data.free_fn) (attach->u.data.data, attach->u.data.free_fn_data); } free(attach); diff --git a/src/libical/icalattach.h b/src/libical/icalattach.h index 43141f6b..a58ed0d3 100644 --- a/src/libical/icalattach.h +++ b/src/libical/icalattach.h @@ -45,13 +45,13 @@ typedef struct icalattach_impl icalattach; /** * @typedef icalattach_free_fn_t - * @brief (*unused*) Function to be called to free the data of an ::icalattach object. + * @brief Function to be called to free the data of an ::icalattach object. * @warning Currently not used * * This function type is used to free the data from an ::icalattach object created * with icalattach_new_from_data(). It is currently not used */ -typedef void (*icalattach_free_fn_t) (unsigned char *data, void *user_data); +typedef void (*icalattach_free_fn_t) (char *data, void *user_data); /** * @brief Create new ::icalattach object from a URL. @@ -88,8 +88,8 @@ LIBICAL_ICAL_EXPORT icalattach *icalattach_new_from_url(const char *url); /** * @brief Create new ::icalattach object from data. * @param data The data to create the ::icalattach from - * @param free_fn (*unused*) The function to free the data - * @param free_fn_data (*unused*) Data to pass to the @a free_fn + * @param free_fn The function to free the data + * @param free_fn_data Data to pass to the @a free_fn * @return An ::icalattach object with the given data * @sa icalattach_unref() * diff --git a/src/libical/icalderivedvalue.c.in b/src/libical/icalderivedvalue.c.in index 140b267e..fad163c1 100644 --- a/src/libical/icalderivedvalue.c.in +++ b/src/libical/icalderivedvalue.c.in @@ -499,6 +499,12 @@ icalvalue *icalvalue_new_binary(const char * v) return (icalvalue*)impl; } +static void free_icalvalue_attach_data(char *data, void *user_data) +{ + _unused(user_data); + free(data); +} + void icalvalue_set_binary(icalvalue *value, const char * v) { struct icalvalue_impl *impl; @@ -512,7 +518,7 @@ void icalvalue_set_binary(icalvalue *value, const char * v) if (impl->data.v_attach) icalattach_unref(impl->data.v_attach); - impl->data.v_attach = icalattach_new_from_data(v, NULL, 0); + impl->data.v_attach = icalattach_new_from_data(strdup(v), free_icalvalue_attach_data, 0); } const char *icalvalue_get_binary(const icalvalue *value) diff --git a/src/libical/icalvalue.c b/src/libical/icalvalue.c index 44541498..31af3724 100644 --- a/src/libical/icalvalue.c +++ b/src/libical/icalvalue.c @@ -428,6 +428,12 @@ static int simple_str_to_double(const char *from, double *result, char **to) return 0; } +static void free_icalvalue_attach_data(char *data, void *user_data) +{ + _unused(user_data); + free(data); +} + static icalvalue *icalvalue_new_from_string_with_error(icalvalue_kind kind, const char *str, icalproperty ** error) { @@ -458,7 +464,7 @@ static icalvalue *icalvalue_new_from_string_with_error(icalvalue_kind kind, { icalattach *attach; - attach = icalattach_new_from_data(str, NULL, 0); + attach = icalattach_new_from_data(strdup(str), free_icalvalue_attach_data, 0); if (!attach) break; diff --git a/src/test/libical-glib/attach.py b/src/test/libical-glib/attach.py index 933274b3..31a04939 100755 --- a/src/test/libical-glib/attach.py +++ b/src/test/libical-glib/attach.py @@ -33,7 +33,7 @@ assert(attach_url.get_is_url() == 1); retrieved_url = attach_url.get_url(); assert(retrieved_url == dummy_url); -attach_data = ICalGLib.Attach.new_from_data(dummy_data, ICalGLib.memory_free_buffer, None); +attach_data = ICalGLib.Attach.new_from_data(dummy_data, None, None); assert(attach_data.get_is_url() == 0); retrieved_data = attach_data.get_data(); assert(retrieved_data == dummy_data); diff --git a/src/test/regression.c b/src/test/regression.c index 2dd5e1c3..6c1acd39 100644 --- a/src/test/regression.c +++ b/src/test/regression.c @@ -3938,11 +3938,23 @@ void test_attach_url() icalcomponent_free(ac); } +static void test_free_attach_data(char *data, void *user_data) +{ + int *pbeen_called = (int *) user_data; + + free(data); + + (*pbeen_called)++; +} + void test_attach_data() { static const char test_icalcomp_str_attachwithdata[] = "BEGIN:VALARM\r\n" "ATTACH;VALUE=BINARY:foofile\r\n" "END:VALARM\r\n"; + static const char test_icalcomp_str_attachwithencodingdata[] = + "BEGIN:VALARM\r\n" "ATTACH;VALUE=BINARY;ENCODING=BASE64:YQECAAACAWEK\r\n" "END:VALARM\r\n"; + int free_been_called = 0; icalattach *attach = icalattach_new_from_data("foofile", NULL, 0); icalcomponent *ac = icalcomponent_new(ICAL_VALARM_COMPONENT); icalproperty *ap = icalproperty_new_attach(attach); @@ -3953,8 +3965,40 @@ void test_attach_data() } str_is("attach data", (const char *) icalattach_get_data(attach), "foofile"); str_is("attach with data", icalcomponent_as_ical_string(ac), test_icalcomp_str_attachwithdata); - icalattach_unref(attach); - icalproperty_free(ap); + + icalattach_unref(attach); + icalcomponent_free(ac); + + ac = icalcomponent_new_from_string(test_icalcomp_str_attachwithdata); + ap = icalcomponent_get_first_property(ac, ICAL_ATTACH_PROPERTY); + attach = icalproperty_get_attach(ap); + str_is("attach data 2", (const char *) icalattach_get_data(attach), "foofile"); + str_is("attach with data 2", icalcomponent_as_ical_string(ac), test_icalcomp_str_attachwithdata); + + icalcomponent_free(ac); + + attach = icalattach_new_from_data(strdup("foofile"), test_free_attach_data, &free_been_called); + ac = icalcomponent_new(ICAL_VALARM_COMPONENT); + ap = icalproperty_new_attach(attach); + + icalcomponent_add_property(ac, ap); + if (VERBOSE) { + printf("%s\n", icalcomponent_as_ical_string(ac)); + } + str_is("attach data 3", (const char *) icalattach_get_data(attach), "foofile"); + str_is("attach with data 3", icalcomponent_as_ical_string(ac), test_icalcomp_str_attachwithdata); + + icalattach_unref(attach); + ok("Free should not be called yet", (!free_been_called)); + icalcomponent_free(ac); + ok("Free should be called now", (free_been_called == 1)); + + ac = icalcomponent_new_from_string(test_icalcomp_str_attachwithencodingdata); + ap = icalcomponent_get_first_property(ac, ICAL_ATTACH_PROPERTY); + attach = icalproperty_get_attach(ap); + str_is("attach data 4", (const char *) icalattach_get_data(attach), "YQECAAACAWEK"); + str_is("attach with data 4", icalcomponent_as_ical_string(ac), test_icalcomp_str_attachwithencodingdata); + icalcomponent_free(ac); } -- cgit v1.2.1