On Wed, Mar 7, 2018 at 11:34 AM, David Woodhouse <dwmw2 at infradead.org> wrote: > > > On Wed, 2018-03-07 at 10:01 +0200, Daniel Lenski wrote: >> What do you prefer? Refactoring the two versions of xmlnode_get_text() >> down to a single function, renaming the gpst.c version, something >> else?? > > Don't know... one option is to ditch it entirely. Some of those cases > where you're just using atoi(s); free(s); after it might be better done > with xmlnode_is_named() and atoi(xml_node->content) perhaps? > > ISTR you saying that referencing node->content doesn't work? I don't > recall the details... Right, it doesn't work. https://stackoverflow.com/questions/10363380/libxml2-can%C2%B4t-get-content-from-node >> > > +/* We behave like CSTP ? create a linked list in vpninfo->cstp_options >> > > + * with the strings containing the information we got from the server, >> > > + * and oc_ip_info contains const copies of those pointers. >> > > + * >> > > + * (unlike version in oncp.c, val is stolen rather than strdup'ed) */ >> > Er... >> > >> > > >> > > + >> > > +static const char *add_option(struct openconnect_info *vpninfo, const char *opt, const char *val) >> > .... make it 'char *val' then, if it's stolen... >> > >> > > >> > > +{ >> > > + struct oc_vpn_option *new = malloc(sizeof(*new)); >> > > + if (!new) >> > > + return NULL; >> > > + >> > > + new->option = strdup(opt); >> > > + if (!new->option) { >> > > + free(new); >> > > + return NULL; >> > > + } >> > > + new->value = strdup(val); >> > >> > ... but I still see a strdup(). And should it *free* 'val' in the error >> > patch if it was going to take ownership of it? >> >> Comment is straight-up wrong and obsolete. We changed this in a >> previous iteration of the patches. Good catch. > > But then you are *using* add_option as the comment describes it: > > for (xml_node = xml_node->children; xml_node; xml_node=xml_node->next) { > if (!xmlnode_get_text(xml_node, "ip-address", &s)) > vpninfo->ip_info.addr = add_option(vpninfo, "ipaddr", s); > > That add_option() call *is* assuming that the 's' value will be stolen, surely? Indeed you're right. Current version is leaking memory every time I do this and then don't free(s) ? :facepalm: This botched add_option() is the source of the memory leaks in the GP version, as I'm now clearly seeing with valgrind. Will resubmit the patch. Thanks, dan
