-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 09/01/12 15:32, Frederic Crozat wrote:
> Le vendredi 06 janvier 2012 à 18:19 +0100, David Sommerseth a écrit :
>> On 06/01/12 17:40, Frederic Crozat wrote:
>>> Le vendredi 06 janvier 2012 à 17:22 +0100, David Sommerseth a
>>> écrit :
>>>> On 12/12/11 18:57, Frederic Crozat wrote:
>>>>> Le lundi 31 octobre 2011 à 22:11 +0100, David Sommerseth a
>>>>> écrit :
>>>>>> On 31/10/11 16:30, Frederic Crozat wrote:
>>>> [...snip...]
>>>>
>>>> Hey again, and thanks for this great rework! I've looked
>>>> through it, and it looks a lot better now. However ...
>>>>
>>>>>> e) Consider to make use of openvpn_execve_check() or
>>>>>> openvpn_execve() instead of the popen() - or make a popen()
>>>>>> variant which got similar error controls to popen() as
>>>>>> openvpn_execve*() functions. I also think that this
>>>>>> execution should honour the --script-security setting (which
>>>>>> is implicitly checked for with openvpn_execve())
>>>>
>>>> I like the approach of the openvpn_popen(). However, there are
>>>> a few things.
>>>>
>>>> + if (pipe(pipe_stdout) == 0) { + pid = fork
>>>> (); + if (pid == (pid_t)0) /* child side */ +
>>>> { +
>>>> close(pipe_stdout[0]); + dup2(pipe_stdout[1],1); +
>>>> execve (cmd, argv, envp); + exit (127); +
>>>> } + else
>>>> if (pid < (pid_t)0) /* fork failed */ + ; ^^^^^ No
>>>> error
>>>> message if fork() fails? And is the (pid_t) typecasting really
>>>> needed?
>>>
>>> Well, I stole the code from openvpn_execve function ;)) I'm ok to
>>> change it, but it would be better for both function to stay
>>> similar, I think.
>>
>> Well, now you got me into looking deeper :) ... First of all, there
>> might be^W^W are code paths in openvpn which isn't pretty. And we
>> should fix up that as we find them.
>>
>> I'm still in the opinion that not reporting that the execution isn't
>> proper behaviour. Even though, it's not likely that fork() fails
>> often, unless the system is low on memory or RLIMIT_NPROC is
>> exceeded. But informing that the execution failed is still
>> important for debugging.
>>
>> I'd say, let's at least add an error message here, using M_ERR.
>> That way, we'll get the errno message returned as well. I'll take
>> care of posting a proper fix for openvpn_execve(). You may
>> disregard my (pid_t) comment for now.
>
> Done.
>
>>>> + else /* parent side */ + { +
>>>> ret=pipe_stdout[0]; + close(pipe_stdout[1]); +
>>>> } +
>>>> }
>>>>
>>>> And:
>>>>
>>>> + else + { + msg (M_WARN, "openvpn_popen: called with
>>>> empty argv"); + }
>>>>
>>>> This should probably be M_FATAL and not M_WARN. If
>>>> openvpn_popen() receives an empty argv array, it most likely is
>>>> due to very bad programming. So halting execution is quite
>>>> appropriate in my eyes.
>>>
>>> Again, I stole this part from openvpn_execve, but I'll change it
>>> to M_FATAL.
>>
>> I'll send a fix for this as well, in regards to openvpn_execve()
>
> Ok :)
>
>>>> In get_console_input_systemd() you do:
>>>>
>>>> + *input = 0;
>>>>
>>>> I think it would be better to do: CLEAR(*input) instead. Just
>>>> setting the first byte to 0, will not solve any leak
>>>> possibilities. An alternative is to use memset() directly,
>>>> which the CLEAR() macro uses (see basic.h:47), but we prefer to
>>>> use CLEAR() over memset() directly.
>>>
>>> done (and I've also fixed some missing spaces in various
>>> location).
>>>
>>> See the new version attached.
>>
>> Goodie! I presume you did some compile and sanity testing before
>> submitting the patch, so that it won't bite our compile farm :) I
>> think we have a F15 or F16 box (which uses systemd) in the farm, so
>> we can enable systemd features on that box.
>
> Well, the initial patch has been shipped as part of openSUSE 12.1 and
> so far, I didn't got any bug report about it, so either it is working
> perfectly or nobody is able to connect home using their VPN and
> therefore can complain ;) I'd tested again attached patch and it is
> still working fine.
I ACKed the patch, and it's just pushed out to master branches on both
- -testing and -stable.
commit 9449e6a9eba30c9ed054f57d630a88c9f087080f
kind regards,
David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iEYEARECAAYFAk8MYRYACgkQDC186MBRfrqV9wCgl0fxhbpvY0NMav2tJkbT75T+
BaYAnRmU345zp+3+KdMlBWaAkXMHmygS
=IFYf
-----END PGP SIGNATURE-----
