Thanks for the bug report! I'm a little uncertain how well the proposed patch works. What happens if you call pskc_build_xml() multiple times? Wouldn't there be many xmlDoc pointers around? And consequently potentially stale pointers if any of them would be free'd?
/Simon David Woodhouse <[email protected]> writes: > When we parse a PSKC file with pskc_parse_from_memory() and subsequently > process the xmlDoc with parse_keycontainer(), we build up a bunch of > pointers from the container to the 'static' strings within the xmlDoc. > > Then we call pskc_build_xml() which builds a *new* xmlDoc and frees the > old one. Leaving lots and lots of stale pointers to the memory we just > freed. > > https://bugzilla.redhat.com/show_bug.cgi?id=1129491 > > The best answer is to fix those stale pointers. We could possibly do > that by calling parse_keycontainer() again in a new 'reparse' mode so > that it doesn't reallocate things (and so that existing pointers remain > valid). There's half an attempt at that in > https://bugzilla.redhat.com/show_bug.cgi?id=1129491#c1 > > Alternatively, we could set the fields in the data structures *as* we > build up the new xmlDoc. But that has issues if we get a failure when > building the new xmlDoc and we'd need to roll back. At least with the > former option we could be fairly sure that parse_keycontainer() really > *shouldn't* fail on the xmlDoc that we just created. > > The third and by far the simplest approach, although less efficient, is > just to keep the original xmlDoc around for the entire lifetime of the > container. That's what this patch does: > > diff --git a/libpskc/build.c b/libpskc/build.c > index 8f0840f..c3e84ad 100644 > --- a/libpskc/build.c > +++ b/libpskc/build.c > @@ -510,7 +510,7 @@ pskc_build_xml (pskc_t * container, char **out, size_t * > len) > > xmlDocSetRootElement (doc, keycont); > > - if (container->xmldoc) > + if (container->xmldoc && container->xmldoc != container->original_xmldoc) > xmlFreeDoc (container->xmldoc); > container->xmldoc = doc; > doc = NULL; > diff --git a/libpskc/internal.h b/libpskc/internal.h > index 674625d..9b36e28 100644 > --- a/libpskc/internal.h > +++ b/libpskc/internal.h > @@ -103,7 +103,7 @@ struct pskc_key > struct pskc > { > /* raw XML */ > - xmlDocPtr xmldoc; > + xmlDocPtr xmldoc, original_xmldoc; > /* Is there a Signature element in xmldoc? */ > int signed_p; > > diff --git a/libpskc/parser.c b/libpskc/parser.c > index 47f8bd6..d75e5ac 100644 > --- a/libpskc/parser.c > +++ b/libpskc/parser.c > @@ -677,6 +677,8 @@ pskc_done (pskc_t * container) > return; > > xmlFreeDoc (container->xmldoc); > + if (container->original_xmldoc != container->xmldoc) > + xmlFreeDoc (container->original_xmldoc); > > for (i = 0; i < container->nkeypackages; i++) > { > @@ -717,7 +719,7 @@ pskc_parse_from_memory (pskc_t * container, size_t len, > const char *buffer) > if (xmldoc == NULL) > return PSKC_XML_ERROR; > > - container->xmldoc = xmldoc; > + container->original_xmldoc = container->xmldoc = xmldoc; > > root = xmlDocGetRootElement (xmldoc); > parse_keycontainer (container, root, &rc);
