corinthia-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Peter Kelly <pmke...@apache.org>
Subject printMissingTag (Branch"odf-filter-attempt2" review)
Date Tue, 12 May 2015 06:51:59 GMT
This code:

    char *printMissingTag(Tag tag)
    {
        char *s = translateXMLEnumName[tag];
        int len = strlen(s)+14+20;
        char *r = malloc(len);
        snprintf(r, len, "%s Missing tag: %s %s",RED, s, RESET);
        return r;
    }

Is likely to be incorrect, because it uses hard-coded numbers and it took me a good while
to figure out where the 14 and 20 were derived from. Getting this wrong can lead to buffer
overflows, which can result in crashes or security vulnerabilities or other problems.

There’s a function called DFFormatString in DFString.h which will basically do the above
for you. So you can just use the following instead:

    char *printMissingTag(Tag tag)
    {
        char *s = translateXMLEnumName[tag];
        return DFFormatString("%s Missing tag: %s %s",RED, s, RESET);
    }

Also, the fact that you’re assigning this to the value attribute of a DFNode object will
lead to a memory leak, because you’re not freeing that memory anywhere. Actually all of
the memory for DFNode objects and the attributes and string values they maintained is supposed
to be allocated by the DFDocument’s own internal memory allocator, which, when you release
the last reference to a DFDocument object, releases all the memory in one go.

There’s a function called DFSetNodeValue which should always be used to set the value of
text nodes. You can see the implementation of this in DFDOM.c; it uses the document’s memory
allocator to make a copy of the string, and then assign that to the node’s value. So if
you were to keep the printMissingTag as it exists above, you would replace the following code
in ODFText.c:

    child->value = printMissingTag(odfChild->tag);

with:

    char *value = printMissingTag(odfChild->tag);
    DFSetNodeValue(child,value);
    free(value);

Note that we free the result of printMissingTag here, as otherwise the memory will leak.

I’d also suggest a different name than “printMissingTag”, since the function itself
doesn’t actually do any printing (it instead returns a string, which the caller can either
print or do something else with, e.g. assigning it to a text node value).

Finally, while the following line:

    child = DFCreateChildElement(htmlNode, 0);

creates an invalid node; you’re using 0 as the tag. Actually you’re creating a text node
here, not an element, so DFCreateChildTextNode would be the appropriate function to use here
instead. Combining this with the above, you could replace the following two lines:

    child = DFCreateChildElement(htmlNode, 0);
    child->value = printMissingTag(odfChild->tag);

with:

    char *value = printMissingTag(odfChild->tag);
    child = DFCreateChildTextNode(htmlNode,value);
    free(value);

—
Dr Peter M. Kelly
pmkelly@apache.org

PGP key: http://www.kellypmk.net/pgp-key <http://www.kellypmk.net/pgp-key>
(fingerprint 5435 6718 59F0 DD1F BFA0 5E46 2523 BAA1 44AE 2966)

> On 10 May 2015, at 8:52 am, Gabriela Gibson <gabriela.gibson@gmail.com> wrote:
> 
> Hi,
> 
> So far I got my branch to produce a list of html nodes (and report on
> still missing stuff).
> 
> This is probably a good point to have a look if the approach I'm using
> here is any good.
> 
> It of course has quite a few warts still, and I think I will need to
> add function pointers to the ODF_to_HTML_key struct to deal with some
> special cases.  If that struct is a good idea that is.
> 
> The branch can be found here:
> 
> https://github.com/apache/incubator-corinthia/commit/c81e68626489b9515e7e8f3a5ce5d38ac8f59af0
> 
> I added the test odt file I was using, plus the current output of the program.
> 
> thanks for looking,
> 
> G
> 
> -- 
> Visit my Coding Diary: http://gabriela-gibson.blogspot.com/


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message