Let icalattach_new_from_data() use the 'free_fn' argument againmcrha/annotations

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'.

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 @@
         <returns type="ICalAttach *" annotation="transfer full" comment="The newly created #ICalAttach from the @url" />
         <comment xml:space="preserve">Create a new #ICalAttach from the url</comment>
     </method>
-    <method name="i_cal_attach_new_from_data" corresponds="icalattach_new_from_data" kind="constructor" since="1.0">
+    <declaration position="body">
+static void icalglib_free_attach_data(gpointer data, gpointer user_data)
+{
+    _unused(user_data);
+    g_free (data);
+}</declaration>
+    <method name="i_cal_attach_new_from_data" corresponds="CUSTOM" kind="constructor" since="1.0">
         <parameter type="const gchar *" name="data" comment="The data used to create the #ICalAttach"/>
         <parameter type="GFunc" name="free_fn" translator="(icalattach_free_fn_t)" annotation="scope call, allow-none" comment="The function used to free the data when the create #ICalAttach is detroyed."/>
         <parameter type="void *" name="free_fn_data" annotation="allow-none" comment="The userdata used for the free function @free_fn"/>
         <returns type="ICalAttach *" annotation="transfer full" comment="The newly created #ICalAttach" />
         <comment xml:space="preserve">Create a new #ICalAttach from the data.</comment>
+        <custom>    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);</custom>
     </method>
     <declaration position="body">
-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);
 }</declaration>
     <method name="i_cal_attach_new_from_bytes" corresponds="CUSTOM" kind="constructor" since="1.0">
         <parameter type="GBytes *" annotation="transfer full" name="bytes" comment="The #GBytes holding the data used to create the #ICalAttach"/>
         <returns type="ICalAttach *" annotation="transfer full" comment="The newly created #ICalAttach" />
-        <comment xml:space="preserve">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.</comment>
+        <comment xml:space="preserve">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).</comment>
         <custom>    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);</custom>
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);
 }