Summary: | WebSocket crash when it receives bad header. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Fumitoshi Ukai <ukai> | ||||||||
Component: | WebCore Misc. | Assignee: | Fumitoshi Ukai <ukai> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Fumitoshi Ukai
2010-04-15 16:29:10 PDT
Created attachment 53487 [details]
Patch
+static String encodeHeaderField(const char* data, size_t len) This function needs a name that makes intended usage clear, something like encodeHeaderFieldForDisplay. Otherwise, one can't help but wonder why it does what it does. But I'm also not sure how much it helps to display garbage headers encoded as JavaScript literal strings. For one thing, this could be natural for a JavaScript function, but HTTP is a distinct field. It's better to tell what is wrong about the header field name or value than dump a string a hex codes. An finally, this may not be an important enough case to to go into all this trouble. + return p; Should this be "return 0" to indicate error? (In reply to comment #2) > +static String encodeHeaderField(const char* data, size_t len) > > This function needs a name that makes intended usage clear, something like > encodeHeaderFieldForDisplay. Otherwise, one can't help but wonder why it does > what it does. Ok. I'll rename it to encodeHeaderFieldForDisplay(). > > But I'm also not sure how much it helps to display garbage headers encoded as > JavaScript literal strings. For one thing, this could be natural for a > JavaScript function, but HTTP is a distinct field. It's better to tell what is > wrong about the header field name or value than dump a string a hex codes. An > finally, this may not be an important enough case to to go into all this > trouble. I see. Make it to dump a string a hex codes. > + return p; > > Should this be "return 0" to indicate error? Oops. I'll fix. Comment on attachment 53487 [details] Patch > I see. Make it to dump a string a hex codes. To re-iterate, I think that it's not great. Ideally, the text should explain what exactly is wrong with the header, and not just dump it in the face of the developer. But it's not a big deal. Marking r- for the "return p" issue. Created attachment 53494 [details]
Patch
Comment on attachment 53494 [details] Patch > CONSOLE MESSAGE: line 0: invalid header name: a5 This looks even more confusing than before. What is invalid about the name "a5"? (In reply to comment #6) > (From update of attachment 53494 [details]) > > CONSOLE MESSAGE: line 0: invalid header name: a5 > > This looks even more confusing than before. What is invalid about the name > "a5"? "a5" only is invalid UTF-8 sequence. Yes, I understand this. But to a Web developer, this would look like an ASCII string "a5" - there is nothing to say or imply that this is hexadecimal. And most Web developers probably don't know how utf-8 works anyway. (In reply to comment #8) > Yes, I understand this. But to a Web developer, this would look like an ASCII > string "a5" - there is nothing to say or imply that this is hexadecimal. And > most Web developers probably don't know how utf-8 works anyway. Ok, so changing console log to " invalid UTF-8 sequence in header name: 0xa5 (in hexadecimal)" is fine? My preferred message would be just "invalid UTF-8 sequence in header name". (In reply to comment #10) > My preferred message would be just "invalid UTF-8 sequence in header name". Does it mean we don't need to dump byte sequence of header name in hexadecimal? IIRC, raw byte sequence might not be outputted correctly in some DRT, so we might need to rebaseline expectations. I think that there is no need to dump the headers, yes. (In reply to comment #12) > I think that there is no need to dump the headers, yes. Ok, so I'll change log message and remove dumping byte sequence. Created attachment 53566 [details]
Patch
Comment on attachment 53566 [details]
Patch
r=me
Committed r57760: <http://trac.webkit.org/changeset/57760> |