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

[Daniel Lenski]

Here is a small patch to fix the comment on add_option…

diff --git a/gpst.c b/gpst.c
index 85987b2..1d5c748 100644
--- a/gpst.c
+++ b/gpst.c
@@ -84,11 +84,10 @@ static int xmlnode_get_text(xmlNode *xml_node,
const char *name, char **var)
     return 0;
 }

-/* We behave like CSTP — create a linked list in vpninfo->cstp_options
+/* We behave like CSTP and ONCP — 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) */
+ */

 static const char *add_option(struct openconnect_info *vpninfo, const
char *opt, const char *val)
 {

On Wed, Mar 7, 2018 at 10:01 AM, Daniel Lenski <dlenski at gmail.com> wrote:
> 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