[PATCH] Terminate decode at end of string and fix mem leak

Miika Turkia miika.turkia at gmail.com
Sat Mar 16 22:12:23 PDT 2013


On Sun, Mar 17, 2013 at 12:16 AM, Dirk Hohndel <dirk at hohndel.org> wrote:
> Miika Turkia <miika.turkia at gmail.com> writes:
>
>> Seems that we have to NULL terminate the buffer for
>> xmlStringLenDecodeEntitites() as otherwise we might end up having extra
>> data at the end of returned buffer.  (Somehow the length parameter is
>> not respected always, even if it is the proper size returned by the
>> zip_fread() - header_skip).
>>
>> Also free the buffer returned by xmlStringLenDecodeEntitites().
>>
>> Signed-off-by: Miika Turkia <miika.turkia at gmail.com>
>> ---
>>  file.c      |    1 +
>>  parse-xml.c |   13 +++++++++----
>>  2 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/file.c b/file.c
>> index b5fba30..8deb2fe 100644
>> --- a/file.c
>> +++ b/file.c
>> @@ -72,6 +72,7 @@ static void zip_read(struct zip_file *file, GError **error, const char *filename
>>               size = read * 3 / 2;
>>               mem = realloc(mem, size);
>>       }
>> +     mem[read] = 0;
>>       parse_xml_buffer(filename, mem, read, &dive_table, error);
>>       free(mem);
>>  }
>> diff --git a/parse-xml.c b/parse-xml.c
>> index b24806b..6dd3094 100644
>> --- a/parse-xml.c
>> +++ b/parse-xml.c
>> @@ -1538,7 +1538,7 @@ static void reset_all(void)
>>   * but once we decode the HTML encoded characters they turn
>>   * into UTF-8 instead. So skip the incorrect encoding
>>   * declaration and decode the HTML encoded characters */
>> -const char *preprocess_divelog_de(const char *buffer)
>> +char *preprocess_divelog_de(const char *buffer)
>
> why remove the const?

We need to free the allocated string, thus I thought working without
const would be better than typecasting const char * to char * on the
free.

>>  {
>>       char *ret = strstr(buffer, "<DIVELOGSDATA>");
>>
>> @@ -1551,17 +1551,22 @@ const char *preprocess_divelog_de(const char *buffer)
>>
>>               return ret;
>>       }
>> -     return buffer;
>> +     return NULL;
>
> Why return NULL in this case...
>
>>  }
>>
>>  void parse_xml_buffer(const char *url, const char *buffer, int size,
>>                       struct dive_table *table, GError **error)
>>  {
>>       xmlDoc *doc;
>> -     const char *res = preprocess_divelog_de(buffer);
>> +     char *res = preprocess_divelog_de(buffer);
>>
>>       target_table = table;
>> -     doc = xmlReadMemory(res, strlen(res), url, NULL, 0);
>> +     if (res) {
>> +             doc = xmlReadMemory(res, strlen(res), url, NULL, 0);
>> +             free(res);
>> +     } else
>> +             doc = xmlReadMemory(buffer, size, url, NULL, 0);
>> +
>
> and then have the if statement here?
>
> I'm missing the point why this is better (and I dislike the asymetric "}
> else" -- if you one side has curly braces, the other one should, too...

We need to free the res only if the process_divelog_de function
allocates new buffer for it. I have attached a different version that
does this by type casting if you prefer that.

miika
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Terminate-decode-at-end-of-string-and-fix-mem-leak.patch
Type: application/octet-stream
Size: 1510 bytes
Desc: not available
URL: <http://lists.hohndel.org/pipermail/subsurface/attachments/20130317/7490baae/attachment-0001.obj>


More information about the subsurface mailing list