On 28/10/16 18:42, Heiko Hund wrote: > The previous implementation had the problem that it was not fully > compatible with printf() and could only detect % format directives > following a space character (0x20). > > It modifies the format string and inserts marks to separate groups > before passing it to the regular printf in libc. The marks are > later used to separate the output string into individual command > line arguments. > > The choice of '\035' as the argument delimiter is based on the > assumption that no "regular" string passed to argv_printf_*() will > ever have to contain that byte (and the fact that it actually is > the ASCII "group separator" control character, which fits its > purpose). > > Signed-off-by: Heiko Hund <heiko.h...@sophos.com> > --- > src/openvpn/argv.c | 203 > ++++++++++++++--------------------- > src/openvpn/argv.h | 4 +- > src/openvpn/route.c | 8 +- > src/openvpn/tun.c | 22 ++-- > tests/unit_tests/openvpn/test_argv.c | 34 +++++- > 5 files changed, 130 insertions(+), 141 deletions(-)
This one was harder to review. Had to use 'git diff --histogram' to
make it easier to read and parse. And there I spotted a few odd things.
In the new argv_prep_format() function:
+ if (!in_token)
+ {
+ ++*count;
+ if (f[0])
+ f[j++] = delim;
+ }
What is the purpose of the f[0] check? At first glance it looks like a
NOOP, as you'd expect f[0] to always have a value > 0. But that is just
until you've parsed something which have been put into f, so I presume
it is some kind of "lock" to not write a delimiter as the first byte in
a string.
Can this whole function be made a bit simpler to understand, as I'm
struggling to fully follow what happens and why? If I spend enough time
digging into this, I will understand it ... but I prefer code which is
easier to understand at first glance, unless the task can't be resolved
with less complexity.
Suggestion: Why not just do a memmove()/memcpy() to the destination
buffer first, then loop through the destination buffer and flip the
in_token flag and replace spaces with delim where needed, but directly
on the destination buffer? With this method, you could even skip the
first byte easily before the loop.
So to argv_printf_arglist():
+ va_copy (tmplist, arglist);
+ len = vsnprintf (NULL, 0, f, tmplist);
+ va_end (tmplist);
+ if (len < 0)
+ goto out;
This is reasonably okay, but would benefit from a comment simply
explaining what you do here. I presume it is a kind of estimated
strlen() operation of the final formatted string.
The vsnprintf(NULL, 0, ...) should also have a little note, as this
requires C99 mode to work - otherwise it will return something
completely unexpected.
Otherwise it looks good. Valgrind tests reports no leaks, t_client.sh
and t_lpback.sh tests are good. BUT t_cltsrv.sh fails, so that is also
something which needs to be fixed. The error is:
Wed Nov 9 21:37:26 2016
Wed Nov 9 21:37:26 2016 openvpn_execve: called with empty argv
Wed Nov 9 21:37:26 2016 Exiting due to fatal error
Reverting this patch makes t_cltsrv.sh succeed, so pretty sure this
patch is the one to blame for this error.
--
kind regards,
David Sommerseth
OpenVPN Technologies, Inc
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
