Mailing List Archive

Hi Florian,

Florian Schlichting wrote:
> Hi,
>
> we've had a request in the Debian bug tracker for a few years to ignore
> trailing spaces when reading values from the configuration file (see
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=595660). Also, there's
> a note in config.c:219 "Needs just a bit work to fix the parser to care
> about ' ' or '\t' after the wanted option..." - and tonight, I felt like
> I could do something about that:
>
>
> --- a/config.c
> +++ b/config.c
> @@ -529,9 +529,16 @@
> configs[config_names[i].nm] = config_names[i].name;
> break;
> }
> - if (configs[config_names[i].nm] == NULL)
> - configs[config_names[i].nm] =
> - strdup(line + strlen(config_names[i].name));
> + /* get option value, skipping leading and trailing whitespace */
> + if (configs[config_names[i].nm] == NULL) {
> + size_t start;
> + for (start = strlen(config_names[i].name); line[start] == ' ' || line[start] == '\t'; start++)
> + ;
> + llen--;
> + for ( ; line[llen] == ' ' || line[llen] == '\t' ; llen--)
> + line[llen] = 0;
> + configs[config_names[i].nm] = strdup(line + start);
> + }
> if (configs[config_names[i].nm] == NULL)
> error(1, errno, "can't allocate memory");
> break;
>
>
> One thing to keep in mind might be people with a password ending in
> blanks, but then that's just asking for trouble...

Umm, I always liked such passwords, really, a bit hard to realize a blank space
in tcpdumps, etc. ;-) But other than that, good job, thanks.

Martin
_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/

On Sat, Apr 14, 2012 at 7:47 AM, Florian Schlichting
<fschlich@zedat.fu-berlin.de> wrote:
> Hi,
>
> we've had a request in the Debian bug tracker for a few years to ignore
> trailing spaces when reading values from the configuration file (see
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=595660). Also, there's
> a note in config.c:219 "Needs just a bit work to fix the parser to care
> about ' ' or '\t' after the wanted option..." - and tonight, I felt like
> I could do something about that:

Hi Florian,
did not tested the patch, but looks good.

About ' ' and '\t' in config file, there is still one item pending,
that would be nice to get fixed too (I'm not complaining your patch,
I'm just highlighting the topic. If someone want propose another
patch, that's good).
All fields config_names[].name with "needsArgument==1" end with a ' '
that is considered part of the name.
This ' ' can be striped away adding a check for a mandatory ' ' or
'\t' after the name.
In this way we add flexibility of using only tabs as separator.

> One thing to keep in mind might be people with a password ending in
> blanks, but then that's just asking for trouble...

What about adding something as below:

> --- a/config.c
> +++ b/config.c
> @@ -529,9 +529,16 @@
>                                        configs[config_names[i].nm] = config_names[i].name;
>                                        break;
>                                }
> -                               if (configs[config_names[i].nm] == NULL)
> -                                       configs[config_names[i].nm] =
> -                                               strdup(line + strlen(config_names[i].name));
> +                                /* get option value, skipping leading and trailing whitespace */
> +                               if (configs[config_names[i].nm] == NULL) {
> +                                    size_t start;
> +                                    for (start = strlen(config_names[i].name); line[start] == ' ' || line[start] == '\t'; start++)
> +                                        ;
> +                                    llen--;
> +                                    for ( ; line[llen] == ' ' || line[llen] == '\t' ; llen--)
> +                                        line[llen] = 0;

+ /* remove optional quotes */
+ if (start != llen && line[start] == '"' &&
line[llen] == '"') {
+ start ++;
+ line[llen--] = 0;
+ }

> +                                    configs[config_names[i].nm] = strdup(line + start);
> +                                }
>                                if (configs[config_names[i].nm] == NULL)
>                                        error(1, errno, "can't allocate memory");
>                                break;

Antonio
_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/

Martin, Antonio,

thanks a lot for your replies. It's encouraging to get positive feedback
so quickly!

> All fields config_names[].name with "needsArgument==1" end with a ' '
> that is considered part of the name.
> This ' ' can be striped away adding a check for a mandatory ' ' or
> '\t' after the name.
> In this way we add flexibility of using only tabs as separator.

Good point. I've added that, plus a line to add a blank when printing
the configuration. And your addition for removing optional quotes. Also,
I hope I got the space/tab thing right this time, so please find the
entire patch attached - it seems to work correctly as far as my testing
goes.

Florian

On Sun, Apr 15, 2012 at 3:59 AM, Florian Schlichting
<fschlich@zedat.fu-berlin.de> wrote:
> Martin, Antonio,
>
> thanks a lot for your replies. It's encouraging to get positive feedback
> so quickly!
>
>> All fields config_names[].name with "needsArgument==1" end with a ' '
>> that is considered part of the name.
>> This ' ' can be striped away adding a check for a mandatory ' ' or
>> '\t' after the name.
>> In this way we add flexibility of using only tabs as separator.
>
> Good point. I've added that, plus a line to add a blank when printing
> the configuration. And your addition for removing optional quotes. Also,
> I hope I got the space/tab thing right this time, so please find the
> entire patch attached - it seems to work correctly as far as my testing
> goes.
>
> Florian
>

Florian,
I'm not at my desk and cannot run tests nor compile. I'll check later...
Does it apply to ./trunk/ or to ./branches/vpnc-nortel/ ? Just to know
,,, I will anyway apply on both with the proper modification.
What is the deadline for including it in wheezy, as you write in another email?

+ /* skip further leading and
trailing whitespace */
+ for (start++; line[start] == '
' || line[start] == '\t'; start++)
+ ;
+ for (llen--; line[llen] == ' '
|| line[llen] == '\t' ; llen--)
+ line[llen] = 0;
Is this part safe in case of *wrong* line made by:
- valid option that need argument (e.g. "IPSec ID")
- followed by just a set of ' ' and '\t' ?
I mean, with a line like "IPSec ID\t\t\t\t" start will be increased
till it points the end of string, then llen will go back till the
first char not blank ('D' in my example).
The value of startt would be higher than llen.
I do not see any critical issue, since spaces are replaced with '\0'.
Please help me to check if I'm missing some case.

Probably it's better to evaluate llen as first, then start. Will force
the example above to end with the same value for start and llen.

- printf("%s%s\n", config_names[i].name,
+ printf("%s%s%s\n", config_names[i].name,
+ config_names[i].needsArgument ? " " : "",
config_names[i].needsArgument ?
config[config_names[i].nm] : "");

I agree using one ' ' as separator. '\t' would work too, but I like
keeping ' ' for backward compatibility.
It's missing the case of argument that starts or ends (or both) with a
space. It would require to be enclosed between double quotes.
There is also the additional case of an argument (password ?) that
starts and ends with double quotes. Also in this case it need to be
put within additional pair of quotes. It's ugly, but it's the only way
to get from "--print-config" a config file that provides same values.
The easier way would be to *always* put quotes, but the result would
break backward compatibility.

Good job.
Thanks,
Antonio
_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/

Antonio,

unfortunately I don't have time to work on this tonight, so just a few
comments:

> Does it apply to ./trunk/ or to ./branches/vpnc-nortel/ ? Just to know

trunk. What's the reason for the separate vpnc-nortel branch, btw, and
is that something Debian would want to ship in addition to the
"standard" vpnc?

> What is the deadline for including it in wheezy, as you write in another email?

According to the current schedule, wheezy is supposed to freeze "in
June". So an upload in late May should be ok, and bugfixes can go in
even after that.

> + /* skip further leading and
> trailing whitespace */
> + for (start++; line[start] == '
> ' || line[start] == '\t'; start++)
> + ;
> + for (llen--; line[llen] == ' '
> || line[llen] == '\t' ; llen--)
> + line[llen] = 0;
> Is this part safe in case of *wrong* line made by:
> - valid option that need argument (e.g. "IPSec ID")
> - followed by just a set of ' ' and '\t' ?
> I mean, with a line like "IPSec ID\t\t\t\t" start will be increased
> till it points the end of string, then llen will go back till the
> first char not blank ('D' in my example).
> The value of startt would be higher than llen.
> I do not see any critical issue, since spaces are replaced with '\0'.
> Please help me to check if I'm missing some case.

Since line is null-terminated and known to start with an option name, I
don't see how the pointers could come to point beyond the start or end
of line. I agree it does look fishy for start to end up higher than
llen, and while llen isn't used any more after that and it feels more
natural to first do the beginning and then the end, this is going to be
a latent bug if someone sometime adds code that does use llen again.

What I just realized is that your example wrong line will currently be
accepted as containing a value, but then the value read in is the empty
string. So after removing optional quotes, there should probably be
another check like

if (start == llen)
error(0, 0, "option '%s' requires a value!", config_names[i].name);


> + printf("%s%s%s\n", config_names[i].name,
> + config_names[i].needsArgument ? " " : "",
> config_names[i].needsArgument ?
> config[config_names[i].nm] : "");

> It's missing the case of argument that starts or ends (or both) with a
> space. It would require to be enclosed between double quotes.
> There is also the additional case of an argument (password ?) that
> starts and ends with double quotes. Also in this case it need to be
> put within additional pair of quotes. It's ugly, but it's the only way
> to get from "--print-config" a config file that provides same values.

I agree.

Florian
_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/

Antonio,

I found a tuit, amended patch attached. Please test it thoroughly!

And are you aware that Ubuntu has patched vpnc (and network manager) to
include the "split DNS" patch that Evan Broder posted here a few weeks
ago, for the upcoming 12.04 LTS release? Even if vpnc-script cannot deal
with the data if network manager can, I think it would be useful to
include.

Florian