[Openswan dev] patch for bug 1067

[Paul Wouters]

On Sun, 25 Oct 2009, D. Hugh Redelmeier wrote:

> A patch for this bug report has been adopted.
> 	https://gsoc.xelerance.com/issues/1067
>
> MCR mentioned that there appeared to be problems with it so I had a
> look.
>
> Systems without popen(3) are broken.  So this workaround does not deserve
> to be first class.  Perhaps #ifdef NO_POPEN.
>
> Worse, apparently there is a popen, but it always returns failure.
> That is crazy!  So crazy that it deserves investigation (but not by
> me).
>
> Checking errno without first checking if there had been an error (i.e.
> f == NULL) is wrong.  Thus this patch introduces a bug into all
> versions of Openswan, not just those without a working popen.

I had changed it slightly after Michael told me about this as well

         f = popen(cmd, "r");

         if (f == NULL)
         {
            /* See bug #1067  Angstrom Linux on a arm7 has no popen() */
            if (errno == ENOSYS) {
                 /* Try system(), though it will not give us output */
                 system(cmd);
                 DBG_log("unable to popen(), falling back to system()");
                 return TRUE;
            }

            loglog(RC_LOG_SERIOUS, "unable to popen %s%s command", verb, verb_suffix);
            signal(SIGCHLD, savesig);
            return FALSE;
         }

> system(3) is not a work-alike for popen.  You cannot just do one in
> place of the other.

Is there a better alternative? Apparently using system() does work better 
then calling the non-implem,ented popen(). It might not be "work-alike"
but it might be "better then noting"?

> The signal handler for SIGCHLD is not restored if the system(3) path
> is used.

I could add that in the above code segment.....

> Unconditionally returning TRUE is wrong.

Returning FALSE and brekaing the tunnel is worse?

> While I'm looking at this code, I might as well ask about some lines
> early in this function.  Feel free to ignore: no bugs are disclosed.
>
>    DBG(DBG_CONTROL, DBG_log("executing %s%s: %s"
>        , verb, verb_suffix, cmd));
>    {
>       char tmp[100];
>       int slen,i;
>       memset(tmp,0,sizeof(tmp));
>       slen=strlen(cmd);
>       DBG(DBG_CONTROL, DBG_log("popen(): cmd is %d chars long", slen));
>       for(i=0; i<slen; i+=80) {
>               strncpy(tmp,&cmd[i],80);
>               DBG(DBG_CONTROL, DBG_log("cmd(%4d):%s:", i, tmp));
>       }
>    }
>
> What's the idea of the braced section?  What does it do that the first
> command does not?  Do commands ever get longer than 80 characters?

I think it might be some naive way of formatting the output to 80 characters.
The commit log does not provide much further information:

commit a712844ce2d9c46a2d243421c48307c72191129f
Author: Paul Wouters <paul at xelerance.com>
Date:   Mon Feb 9 22:27:29 2009 -0500

     The various do_command's used a hardcoded 1536, which some people were
     hitting. Note that there is also duplicate code for sending the
     environment variables from pluto to the updown command. The common code
     is put in fmt_common_shell_out(), but not all stacks are using this yet.

     Also do_command_linux was confusing, since klips was not using it but
     netkey was. It has been renamed netkey_do_command and moved and now uses
     fmt_common_shell_out().

     invoke_command() has some added debugging related to the size of the
     environment passed.

     Patch by Carsten Schlote <c.schlote at konzeptpark.de>


> Here's a simpler version of the braced code, just in case there is
> some point to this.  UNTESTED!
>
>    DBG(DBG_CONTROL,  {
>       int slen = strlen(cmd);
>       int i;
>
>       DBG_log("popen(): cmd is %d chars long", slen);
>       for(i=0; i<slen; i+=80)
>               DBG_log("cmd(%4d):%.80s:", i, cmd+i);
>       });

We should probably just remove the 80 char formatting.....

Paul