code review 4746042: http: sniffing algorithm.

http: sniffing algorithm.

This follows draft-ietf-websec-mime-sniff-03 in its intent,
though not its algorithmic specification.

[dsymonds, 2011-07-15 15:40:45]

Hello rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2011-07-19 17:36:07]

http://codereview.appspot.com/4746042/diff/8003/src/pkg/http/server.go
File src/pkg/http/server.go (right):

http://codereview.appspot.com/4746042/diff/8003/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:363: if ctype != "" {
can delete

http://codereview.appspot.com/4746042/diff/8003/src/pkg/http/sniff.go
File src/pkg/http/sniff.go (right):

http://codereview.appspot.com/4746042/diff/8003/src/pkg/http/sniff.go#newcode31
src/pkg/http/sniff.go:31: off := 0
i suggest

type sniffSig interface {
    match(data []byte, firstNonWS int) string
}

with an implementation for html and an 
implementation for exact mask, and an
implementation for text.

then your signature list can be pure data
instead of needing to be constructed at 
init time, and more important, it separates
the html concerns from the binary concerns.

for _, sig := range sniffSigs {
    if ct := sig.match(data, firstNonWS); ct != "" {
        return ct
    }
}

http://codereview.appspot.com/4746042/diff/8003/src/pkg/http/sniff.go#newcode126
src/pkg/http/sniff.go:126: exactSig("FF D8 FF", "image/jpeg"),
is this supposed to be \xFF\xD8\xFF ?

[dsymonds, 2011-07-20 02:11:18]

PTAL

http://codereview.appspot.com/4746042/diff/8003/src/pkg/http/server.go
File src/pkg/http/server.go (right):

http://codereview.appspot.com/4746042/diff/8003/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:363: if ctype != "" {
On 2011/07/19 17:36:07, rsc wrote:
> can delete

Done.

http://codereview.appspot.com/4746042/diff/8003/src/pkg/http/sniff.go
File src/pkg/http/sniff.go (right):

http://codereview.appspot.com/4746042/diff/8003/src/pkg/http/sniff.go#newcode31
src/pkg/http/sniff.go:31: off := 0
On 2011/07/19 17:36:07, rsc wrote:
> i suggest
> 
> type sniffSig interface {
>     match(data []byte, firstNonWS int) string
> }
> 
> with an implementation for html and an 
> implementation for exact mask, and an
> implementation for text.

Done, but with s/text/masked/.

Text can't be a sniffSig in the list if we want to match MP4 in the future.

http://codereview.appspot.com/4746042/diff/8003/src/pkg/http/sniff.go#newcode126
src/pkg/http/sniff.go:126: exactSig("FF D8 FF", "image/jpeg"),
On 2011/07/19 17:36:07, rsc wrote:
> is this supposed to be \xFF\xD8\xFF ?

Oops, yes.

[rsc, 2011-07-20 13:10:55]

> Text can't be a sniffSig in the list if we want to match MP4 in the
> future.

Why not?  Doesn't it just go after text?

[rsc, 2011-07-20 13:11:09]

On Wed, Jul 20, 2011 at 09:10, Russ Cox <rsc@golang.org> wrote:
>> Text can't be a sniffSig in the list if we want to match MP4 in the
>> future.
>
> Why not?  Doesn't it just go after text?

Sorry, I meant doesn't text just go after mp4?

[rsc, 2011-07-20 13:15:08]

Still confused about why text isn't a sniffer.

http://codereview.appspot.com/4746042/diff/17001/src/pkg/http/sniff.go
File src/pkg/http/sniff.go (right):

http://codereview.appspot.com/4746042/diff/17001/src/pkg/http/sniff.go#newcode21
src/pkg/http/sniff.go:21: // TODO(dsymonds): This follows the draft spec,
Can delete.  Looks fine to me.

http://codereview.appspot.com/4746042/diff/17001/src/pkg/http/sniff.go#newcod...
src/pkg/http/sniff.go:124: func (e exactSig) match(data []byte, firstNonWS int)
string {
cheaper, more common to make these * methods

[dsymonds, 2011-07-20 13:28:05]

Yeah, don't know what I was thinking about with text not being possible. I think
I was confused that MP4 might be really complex somehow. Anyway, it's done.

http://codereview.appspot.com/4746042/diff/17001/src/pkg/http/sniff.go
File src/pkg/http/sniff.go (right):

http://codereview.appspot.com/4746042/diff/17001/src/pkg/http/sniff.go#newcode21
src/pkg/http/sniff.go:21: // TODO(dsymonds): This follows the draft spec,
On 2011/07/20 13:15:09, rsc wrote:
> Can delete.  Looks fine to me.

Done.

http://codereview.appspot.com/4746042/diff/17001/src/pkg/http/sniff.go#newcod...
src/pkg/http/sniff.go:124: func (e exactSig) match(data []byte, firstNonWS int)
string {
On 2011/07/20 13:15:09, rsc wrote:
> cheaper, more common to make these * methods

Done for exactSig and maskedSig. The others don't need it.

[rsc, 2011-07-20 13:56:04]

LGTM

[rsc, 2011-07-20 15:00:02]

http://codereview.appspot.com/4746042/diff/22002/src/pkg/http/sniff.go
File src/pkg/http/sniff.go (right):

http://codereview.appspot.com/4746042/diff/22002/src/pkg/http/sniff.go#newcode21
src/pkg/http/sniff.go:21: // Index of the first non-whitespace byte in data.
please insert

if len(data) > sniffLen {
    data = data[:sniffLen]
}

so that there is no reason for calling code to try
to be clever and send us more data.

[dsymonds, 2011-07-20 22:38:01]

http://codereview.appspot.com/4746042/diff/22002/src/pkg/http/sniff.go
File src/pkg/http/sniff.go (right):

http://codereview.appspot.com/4746042/diff/22002/src/pkg/http/sniff.go#newcode21
src/pkg/http/sniff.go:21: // Index of the first non-whitespace byte in data.
On 2011/07/20 15:00:02, rsc wrote:
> please insert
> 
> if len(data) > sniffLen {
>     data = data[:sniffLen]
> }
> 
> so that there is no reason for calling code to try
> to be clever and send us more data.

Done.

[dsymonds, 2011-07-20 22:38:44]

*** Submitted as http://code.google.com/p/go/source/detail?r=6675f47ceacf ***

http: sniffing algorithm.

This follows draft-ietf-websec-mime-sniff-03 in its intent,
though not its algorithmic specification.

R=rsc
CC=golang-dev
http://codereview.appspot.com/4746042