[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