On Wed, 2018-03-07 at 10:01 +0200, Daniel Lenski wrote:
> What do you prefer? Refactoring the two versions of xmlnode_get_text()
> down to a single function, renaming the gpst.c version, something
> else??
Don't know... one option is to ditch it entirely. Some of those cases
where you're just using atoi(s); free(s); after it might be better done
with xmlnode_is_named() and atoi(xml_node->content) perhaps??
ISTR you saying that referencing node->content doesn't work? I don't
recall the details...
> > > +/* 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.
But then you are *using* add_option as the comment describes it:
for (xml_node = xml_node->children; xml_node; xml_node=xml_node->next) {
if (!xmlnode_get_text(xml_node, "ip-address", &s))
vpninfo->ip_info.addr = add_option(vpninfo, "ipaddr", s);
That add_option() call *is* assuming that the 's' value will be stolen, surely?
Maybe we should pass the xmlNode into add_option() not a string? Then
it's nice and simple. And more xmlnode_get_text() invocations can turn
into simple xmlnode_is_named()? The above code becomes
for (xml_node = xml_node->children; xml_node; xml_node=xml_node->next) {
if (!xmlnode_is_named(xml_node, "ip-address"))
vpninfo->ip_info.addr = add_option(vpninfo, "ipaddr", xml_node);
> >
> > 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.
#include <stdio.h>
#include <string.h>
int main(void)
{
const char *pre_prompt = "var respMsg = \"";
char *attack = "var respMsg = \";\n";
char *start, *end = attack;
start = end+strlen(pre_prompt);
end = strchr(start, '\n');
if (!end || end[-1] != ';' || end[-2] != '"') {
printf("no match\n");
return 1;
}
printf("end is %p, start %p, end-start-2 %ld\n",
???????end, start, (long)(end-start-2));
return 0;
}
$ ./f
end is 0x400758, start 0x400757, end-start-2 -1
-------------- 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/20180307/966d8a17/attachment-0001.bin>
