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

[David Woodhouse]

Thanks for tidying this up. Pushed to my gpst branch with one fixup so
far, still reading through...

On Sun, 2018-03-04 at 11:31 +0200, Daniel Lenski wrote:
> 
> +/* similar to auth.c's xmlnode_get_text, including that *var should be freed by the caller,
> +   but without the hackish param / %s handling that Cisco needs. And without freeing up
> +   the old contents of *var, which is likely to lead to bugs? */

Yeah... the way you're using it in gpst.c does lend mean that it's
slightly more convenient to *not* free the previous contents. Otherwise
various call sites would have to explicitly set s=NULL after
xmlnode_get_text();add_option().

I'm slightly concerned about having different semantics for the same
(or similar) function in different files though; that is *also* likely
to lead to bugs.

> +static int xmlnode_get_text(xmlNode *xml_node, const char *name, char **var)
> +{
> +       char *str;
> +
> +       if (name && !xmlnode_is_named(xml_node, name))
> +               return -EINVAL;
> +
> +       str = (char *)xmlNodeGetContent(xml_node);
> +       if (!str)
> +               return -ENOENT;
> +
> +       *var = str;
> +       return 0;
> +}
> +
> +/* 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?

Unless I'm being stupid, this should be a memory leak. Have you run in
valgrind with --leak-check=full? Please do.


> +       new->next = vpninfo->cstp_options;
> +       vpninfo->cstp_options = new;
> +
> +       return new->value;
> +}
> +

Also, in parse_javascript(), consider an input line which looks like:

   var respMsg = ";

When you set '*prompt = strndup(start, end-start-2);

... what is the value of 'end-start-2'?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5213 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/openconnect-devel/attachments/20180306/098e76c8/attachment.bin>