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

[Daniel Lenski]

On Tue, Mar 6, 2018 at 11:40 AM, David Woodhouse <dwmw2 at infradead.org> wrote:
> 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.

What do you prefer? Refactoring the two versions of xmlnode_get_text()
down to a single function, renaming the gpst.c version, something
else…?

>
>> +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?

Comment is straight-up wrong and obsolete. We changed this in a
previous iteration of the patches. Good catch.

> 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'?

0 in that case. I've already verified that end[-1] == ';' && end[-2]
== '"' before getting to this point, so it's not possible for it to be
negative.

-Dan