SIGPIPE from syntax highlighting filter if highlight is not available

SIGPIPE from syntax highlighting filter if highlight is not available

[vz-cgit]

 Hello,

 As this is the first time I'm posting here, let me start by thanking you
for developing cgit! I'm using it since quite some time under Debian
(currently Jessie) and it works very well but recently I've mistakenly
removed highlight package because I thought it was redundant with other
similar tools and completely forgot that cgit depended on it.

 Since then, viewing any files in cgit didn't work, which was probably only
to be expected, but viewing sufficiently large files resulted in 500 server
error due to, after debugging, SIGPIPE received but not handled by cgit
when reading from the filter subprocess.

 I'm not sure what exactly should be done here: maybe nothing, and people
should be just more careful with uninstalling dependencies. But I think
ideally cgit should give an error indicating that filter execution failed
and I'd at least expect it not to just die without any further information.

 Thanks again for cgit!
VZ
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20160929/135df564/attachment.asc>

SIGPIPE from syntax highlighting filter if highlight is not available

[john]

On Thu, Sep 29, 2016 at 04:38:41PM +0200, Vadim Zeitlin wrote:
>  As this is the first time I'm posting here, let me start by thanking you
> for developing cgit! I'm using it since quite some time under Debian
> (currently Jessie) and it works very well but recently I've mistakenly
> removed highlight package because I thought it was redundant with other
> similar tools and completely forgot that cgit depended on it.
> 
>  Since then, viewing any files in cgit didn't work, which was probably only
> to be expected, but viewing sufficiently large files resulted in 500 server
> error due to, after debugging, SIGPIPE received but not handled by cgit
> when reading from the filter subprocess.
> 
>  I'm not sure what exactly should be done here: maybe nothing, and people
> should be just more careful with uninstalling dependencies. But I think
> ideally cgit should give an error indicating that filter execution failed
> and I'd at least expect it not to just die without any further information.

If we ignore SIGPIPE we should just hit the die_errno() in
html.c::html_raw() which will say "write error on html output".  That's
not really as useful as it could be, since that code doesn't care
whether or not it's writing via a filter.

I'm not sure if we want to start keeping track of whether we're writing
through a filter, but some form of error message rather than a silent
failure does seem like an improvment.  I think we just need to ignore
SIGPIPE to make that happen.

[PATCH] cgit: ignore SIGPIPE

[john]

We check the return value from write() and die with an error message, so
the only effects of using the default SIGPIPE handler is to suppress the
error message and change the exit status.

Web servers tend to ignore the exit code, so it is likely to be more
helpful to administrators if we print some form of error message with a
more generic exit status rather than die silently with
WTERMSIG() == SIGPIPE.

Signed-off-by: John Keeping <john at keeping.me.uk>
---
On Sat, Oct 01, 2016 at 12:33:00PM +0100, John Keeping wrote:
> On Thu, Sep 29, 2016 at 04:38:41PM +0200, Vadim Zeitlin wrote:
> >  As this is the first time I'm posting here, let me start by thanking you
> > for developing cgit! I'm using it since quite some time under Debian
> > (currently Jessie) and it works very well but recently I've mistakenly
> > removed highlight package because I thought it was redundant with other
> > similar tools and completely forgot that cgit depended on it.
> > 
> >  Since then, viewing any files in cgit didn't work, which was probably only
> > to be expected, but viewing sufficiently large files resulted in 500 server
> > error due to, after debugging, SIGPIPE received but not handled by cgit
> > when reading from the filter subprocess.
> > 
> >  I'm not sure what exactly should be done here: maybe nothing, and people
> > should be just more careful with uninstalling dependencies. But I think
> > ideally cgit should give an error indicating that filter execution failed
> > and I'd at least expect it not to just die without any further information.
> 
> If we ignore SIGPIPE we should just hit the die_errno() in
> html.c::html_raw() which will say "write error on html output".  That's
> not really as useful as it could be, since that code doesn't care
> whether or not it's writing via a filter.
> 
> I'm not sure if we want to start keeping track of whether we're writing
> through a filter, but some form of error message rather than a silent
> failure does seem like an improvment.  I think we just need to ignore
> SIGPIPE to make that happen.

Something like this.

 cgit.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/cgit.c b/cgit.c
index 2f29aa6..1a94144 100644
--- a/cgit.c
+++ b/cgit.c
@@ -1031,6 +1031,9 @@ int cmd_main(int argc, const char **argv)
 	const char *path;
 	int err, ttl;
 
+	if (signal(SIGPIPE, SIG_IGN) == SIG_ERR)
+		fprintf(stderr, "cgit warning: failed to ignore SIGPIPE\n");
+
 	cgit_init_filters();
 	atexit(cgit_cleanup_filters);
 
-- 
2.10.0.278.g4f427b1

[PATCH] cgit: ignore SIGPIPE

[Jason]

Hey John,

I see the utility of this with something like a highlight filter gone
bad. But is it safe to do this in the context of an authentication
filter? What's the failure behavior like once this patch is applied?

Jason

[PATCH] cgit: ignore SIGPIPE

[john]

On Sat, Oct 01, 2016 at 10:52:11PM +0200, Jason A. Donenfeld wrote:
> I see the utility of this with something like a highlight filter gone
> bad. But is it safe to do this in the context of an authentication
> filter? What's the failure behavior like once this patch is applied?

If we ignore SIGPIPE then we get an EPIPE error from write(2); in both
places that use write(2) we die if this happens.

So I think the only changes are our exit code and the fact that with
this patch we print an error message rather than exiting silently.