Re: [bug-gettext] [PATCH 0/6] Fix various memory leaks in libgettextpo

[Daiki Ueno]

Daiki Ueno <address@hidden> writes:

>>   Copy string from shared buffer
>>   Fix memory leak: po_callback_comment_dispatcher does not take
>>     ownership of the string passed into it
>
> These two patches look contradicting each other.  If we don't copy the
> COMMENT string with the first patch, we don't need to free it in the
> second patch, right?
>
>>   Fix memory leak: do not duplicate msgid_plural in message_alloc
>>   Fix memory leak: free msgctxt in message_free
>
> These patches too.  Would it be a problem if we always strdup all the
> fields of message_ty?
>
>>   Fix memory leak: string_list_append duplicates the string
>
> I'd rather prefer to free after string_list_append rather than adding
> the nodup variant of string_list_append.

I've just tried by myself and only the attached fix was needed.  Am I
missing the something?  I used the following code to test:

--8<---------------cut here---------------start------------->8---
#include <errno.h>
#include <gettext-po.h>
#include <stdlib.h>

static void test_xerror (int severity,
                         po_message_t message,
                         const char *filename, size_t lineno, size_t column,
                         int multiline_p, const char *message_text)
{
}

static void test_xerror2 (int severity,
                          po_message_t message1,
                          const char *filename1, size_t lineno1, size_t column1,
                          int multiline_p1, const char *message_text1,
                          po_message_t message2,
                          const char *filename2, size_t lineno2, size_t column2,
                          int multiline_p2, const char *message_text2)
{
}

int
main (int argc, char **argv)
{
  struct po_xerror_handler handler = {
    .xerror = test_xerror,
    .xerror2 = test_xerror2
  };

  const char *filename = argv[1];
  po_file_t file = po_file_read (filename, &handler);

  if (file == NULL)
    error (EXIT_FAILURE, errno, "couldn't open the PO file %s", filename);

  po_file_free (file);

  return EXIT_SUCCESS;
}
--8<---------------cut here---------------end--------------->8---

>From 63aa81c701093aadddd5ace0c6919e0bc4a35034 Mon Sep 17 00:00:00 2001
From: Daiki Ueno <address@hidden>
Date: Thu, 29 Aug 2013 12:33:37 +0900
Subject: [PATCH] po-gram: fix memory leaks

---
 gettext-tools/src/ChangeLog     |  9 +++++++++
 gettext-tools/src/po-gram-gen.y | 17 ++++++++++++-----
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/gettext-tools/src/ChangeLog b/gettext-tools/src/ChangeLog
index 7a8872f..0d2f4c6 100644
--- a/gettext-tools/src/ChangeLog
+++ b/gettext-tools/src/ChangeLog
@@ -1,3 +1,12 @@
+2013-08-29  Daiki Ueno  <address@hidden>
+
+       * po-gram-gen.y (message): Free memory allocated in
+       msgid_pluralform.
+       (string_list): Free memory allocated for STRING.
+       (prev_string_list): Free memory allocated for PREV_STRING.
+       Reported by Alexander Potashev in
+       <https://lists.gnu.org/archive/html/bug-gettext/2013-08/msg00043.html>.
+
 2013-08-13  Miguel Angel Arruga Vivas  <address@hidden>
 
        * x-glade.c (start_element_glade1): Use extract_all variable.
diff --git a/gettext-tools/src/po-gram-gen.y b/gettext-tools/src/po-gram-gen.y
index bdc568a..8f888bd 100644
--- a/gettext-tools/src/po-gram-gen.y
+++ b/gettext-tools/src/po-gram-gen.y
@@ -220,11 +220,14 @@ message
                   check_obsolete ($1, $3);
                   check_obsolete ($1, $4);
                   if (!$1.obsolete || pass_obsolete_entries)
-                    do_callback_message ($1.ctxt, string2, &$1.pos, $3.string,
-                                         $4.rhs.msgstr, $4.rhs.msgstr_len, 
&$4.pos,
-                                         $1.prev_ctxt,
-                                         $1.prev_id, $1.prev_id_plural,
-                                         $1.obsolete);
+                    {
+                      do_callback_message ($1.ctxt, string2, &$1.pos, 
$3.string,
+                                           $4.rhs.msgstr, $4.rhs.msgstr_len, 
&$4.pos,
+                                           $1.prev_ctxt,
+                                           $1.prev_id, $1.prev_id_plural,
+                                           $1.obsolete);
+                      free ($3.string);
+                    }
                   else
                     {
                       free_message_intro ($1);
@@ -411,6 +414,7 @@ string_list
                 {
                   string_list_init (&$$.stringlist);
                   string_list_append (&$$.stringlist, $1.string);
+                  free ($1.string);
                   $$.pos = $1.pos;
                   $$.obsolete = $1.obsolete;
                 }
@@ -419,6 +423,7 @@ string_list
                   check_obsolete ($1, $2);
                   $$.stringlist = $1.stringlist;
                   string_list_append (&$$.stringlist, $2.string);
+                  free ($2.string);
                   $$.pos = $1.pos;
                   $$.obsolete = $1.obsolete;
                 }
@@ -429,6 +434,7 @@ prev_string_list
                 {
                   string_list_init (&$$.stringlist);
                   string_list_append (&$$.stringlist, $1.string);
+                  free ($1.string);
                   $$.pos = $1.pos;
                   $$.obsolete = $1.obsolete;
                 }
@@ -437,6 +443,7 @@ prev_string_list
                   check_obsolete ($1, $2);
                   $$.stringlist = $1.stringlist;
                   string_list_append (&$$.stringlist, $2.string);
+                  free ($2.string);
                   $$.pos = $1.pos;
                   $$.obsolete = $1.obsolete;
                 }
-- 
1.8.3.1

Regards,
-- 
Daiki Ueno