[PATCH 2/5] add PAN GlobalProtect protocol support (HTTPS tunnel only)

[Daniel Lenski]

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