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>
