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