passt.c: incorrect signal() return value check

passt.c: incorrect signal() return value check

[Valtteri Vuorikoski]

In current master, passt.c:main() has an incorrect check on signal()
return value:

               if (sigaction(SIGCHLD, &sa, NULL) || signal(SIGPIPE, SIG_IGN))
                       die("Couldn't install signal handlers");

signal() returns SIG_ERR on failure or the previous setting if present. If
a setting has been inherited from the parent (as is the case when
starting from systemd with the default setting of IgnoreSIGPIPE=yes),
the check will fail.

The if statement should check for SIG_ERR for the SIGPIPE case, or
alternatively switch to sigaction() since that's used for everything
else in the code.

 -Valtteri
 (please cc on replies)
 

Re: passt.c: incorrect signal() return value check

[Stefano Brivio]

Hi Valtteri,

On Thu, 13 Apr 2023 13:24:34 +0300
Valtteri Vuorikoski <vuori@notcom.org> wrote:

> In current master, passt.c:main() has an incorrect check on signal()
> return value:
> 
>                if (sigaction(SIGCHLD, &sa, NULL) || signal(SIGPIPE, SIG_IGN))
>                        die("Couldn't install signal handlers");
> 
> signal() returns SIG_ERR on failure or the previous setting if present. If
> a setting has been inherited from the parent (as is the case when
> starting from systemd with the default setting of IgnoreSIGPIPE=yes),
> the check will fail.

Right, thanks for reporting this.

By the way, feel free to share your systemd unit files, I guess we
could add them under contrib/ in case they are useful for somebody.

> The if statement should check for SIG_ERR for the SIGPIPE case, or
> alternatively switch to sigaction() since that's used for everything
> else in the code.

I guess we could just check SIG_ERR from signal(), it looks a bit
simpler, and perhaps split this into two cases (a failure on signal()
doesn't mean we "[c]ouldn't install signal handlers").

Will you post a patch? It's trivial, so if it's unnecessary effort for
you I can also go ahead and do it.

>  -Valtteri
>  (please cc on replies)

-- 
Stefano

Re: passt.c: incorrect signal() return value check

[Valtteri Vuorikoski]

Hi, thanks for the fast reply.

On Thu, Apr 13, 2023 at 01:25:05PM +0200, Stefano Brivio wrote:
> > signal() returns SIG_ERR on failure or the previous setting if present. If
> > a setting has been inherited from the parent (as is the case when
> > starting from systemd with the default setting of IgnoreSIGPIPE=yes),
> > the check will fail.
> 
> Right, thanks for reporting this.
> 
> By the way, feel free to share your systemd unit files, I guess we
> could add them under contrib/ in case they are useful for somebody.

I'll see if I can put together some kind of readable version for a
patch. This is a somewhat intricate setup to get a specific
constellation of painful legacy services running with 1.x series
Podman under systemd, but the "1.x series Podman under systemd" part
might be useful to someone I guess.

> I guess we could just check SIG_ERR from signal(), it looks a bit
> simpler, and perhaps split this into two cases (a failure on signal()
> doesn't mean we "[c]ouldn't install signal handlers").
> 
> Will you post a patch? It's trivial, so if it's unnecessary effort for
> you I can also go ahead and do it.

If you can just push the "== SIG_ERR" fix please do, my only tree is
currently on another machine.

 -Valtteri
 

Re: passt.c: incorrect signal() return value check

[Stefano Brivio]

On Thu, 13 Apr 2023 17:47:27 +0300
Valtteri Vuorikoski <vuori@notcom.org> wrote:

> Hi, thanks for the fast reply.
> 
> On Thu, Apr 13, 2023 at 01:25:05PM +0200, Stefano Brivio wrote:
> > > signal() returns SIG_ERR on failure or the previous setting if present. If
> > > a setting has been inherited from the parent (as is the case when
> > > starting from systemd with the default setting of IgnoreSIGPIPE=yes),
> > > the check will fail.  
> > 
> > Right, thanks for reporting this.
> > 
> > By the way, feel free to share your systemd unit files, I guess we
> > could add them under contrib/ in case they are useful for somebody.  
> 
> I'll see if I can put together some kind of readable version for a
> patch. This is a somewhat intricate setup to get a specific
> constellation of painful legacy services running with 1.x series
> Podman under systemd, but the "1.x series Podman under systemd" part
> might be useful to someone I guess.

Oh, I didn't know it would be so specific. I'm not sure then -- your
call.

> > I guess we could just check SIG_ERR from signal(), it looks a bit
> > simpler, and perhaps split this into two cases (a failure on signal()
> > doesn't mean we "[c]ouldn't install signal handlers").
> > 
> > Will you post a patch? It's trivial, so if it's unnecessary effort for
> > you I can also go ahead and do it.  
> 
> If you can just push the "== SIG_ERR" fix please do, my only tree is
> currently on another machine.

Sure, patch posted.

-- 
Stefano