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