* SIGPIPE from syntax highlighting filter if highlight is not available @ 2016-09-29 14:38 vz-cgit 2016-10-01 11:33 ` john 0 siblings, 1 reply; 5+ messages in thread From: vz-cgit @ 2016-09-29 14:38 UTC (permalink / raw) 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> ^ permalink raw reply [flat|nested] 5+ messages in thread
* SIGPIPE from syntax highlighting filter if highlight is not available 2016-09-29 14:38 SIGPIPE from syntax highlighting filter if highlight is not available vz-cgit @ 2016-10-01 11:33 ` john 2016-10-01 11:42 ` [PATCH] cgit: ignore SIGPIPE john 0 siblings, 1 reply; 5+ messages in thread From: john @ 2016-10-01 11:33 UTC (permalink / raw) 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] cgit: ignore SIGPIPE 2016-10-01 11:33 ` john @ 2016-10-01 11:42 ` john 2016-10-01 20:52 ` Jason 0 siblings, 1 reply; 5+ messages in thread From: john @ 2016-10-01 11:42 UTC (permalink / raw) 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] cgit: ignore SIGPIPE 2016-10-01 11:42 ` [PATCH] cgit: ignore SIGPIPE john @ 2016-10-01 20:52 ` Jason 2016-10-02 10:45 ` john 0 siblings, 1 reply; 5+ messages in thread From: Jason @ 2016-10-01 20:52 UTC (permalink / raw) 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] cgit: ignore SIGPIPE 2016-10-01 20:52 ` Jason @ 2016-10-02 10:45 ` john 0 siblings, 0 replies; 5+ messages in thread From: john @ 2016-10-02 10:45 UTC (permalink / raw) 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-10-02 10:45 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-29 14:38 SIGPIPE from syntax highlighting filter if highlight is not available vz-cgit 2016-10-01 11:33 ` john 2016-10-01 11:42 ` [PATCH] cgit: ignore SIGPIPE john 2016-10-01 20:52 ` Jason 2016-10-02 10:45 ` john
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).