HTTP::Server::PSGI crash when header value is utf8 string #353

Closed
vlet opened this Issue Nov 27, 2012 · 10 comments

4 participants

@vlet

$ plackup -E deployment -s HTTP::Server::PSGI -e 'sub {[200,[x=>"\x{100}"],[""]]}' &

$ curl http://127.0.0.1:5000/
Wide character in syswrite at /usr/lib64/perl5/IO/Handle.pm line 478.
curl: (52) Empty reply from server
[1] + exit 255 plackup -E deployment -s Standalone -e 'sub {[200,[x=>"\x{100}"],[""]]}'

@miyagawa
plack member
@doy
plack member

@miyagawa It actually isn't a violation as written currently - the PSGI specification states that the body must be encoded, but doesn't say anything about the headers. This should probably be clarified in the spec.

@miyagawa
plack member

Good catch. Should be addressed in the PSGI spec then.

@vlet

There is a crash even with Lint middleware enabled (-E development)

@yko

Does it worth to cover this case in Lint middleware, similar to existing check you mentioned?

Something similar to this check: https://metacpan.org/source/MIYAGAWA/Plack-1.0013/lib/Plack/Middleware/Lint.pm#L155
But on headers element: $res->[1]

@miyagawa
plack member

Yeah, Lint could probably check that as well, once we clarify it in the PSGI spec.

I've been wondering if it's worth adding the Encode::_utf8_off in HTTP::Server::PSGI's syswrite independently. Because unlike normal write/print which merely gives the "Wide strings in print" warning, syswrite crashes with strings with UTF-8 flags, and there's no safer way to do it except Encode::_utf8_off as far as I know.

@doy
plack member

I think covering it in Lint is probably sufficient - everything else is supposed to be able to just assume valid PSGI, isn't it? If fixing it in HTTP::Server::PSGI is actually important, seems like sticking the syswrite inside an eval would be better (along with warn $@ or whatever), since we don't really want exceptions at all at that stage. Encode::_utf8_off seems like the wrong solution here.

@miyagawa
plack member

Yeah, everyone else can assume it's valid PSGI, and there's no need for individual servers to deal with it.

I just thought this crash is pretty severe, given that if this were a plain write() it would've been just warnings. Yes, catching exceptions with eval is probably more "correct" approach, but because it's already in an alarm timeout, implementing could get a bit tricky, while force dropping the flag makes it behave more like a plain write.

But if a patch can be made to work with eval/catch that'd probably be better. (I have no idea why syswrite complains about the flag while write/print doesn't)

@vlet

#429 also fixed that issue

@miyagawa
plack member

Yep, fixed by #429

@miyagawa miyagawa closed this Oct 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment