| Message ID | 20170227201456.31814-1-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 02/27/2017 02:14 PM, Daniel P. Berrange wrote: > According to RFC7230 Section 3.2, header field name is case-insensitive. > Convert the header data into all lowercase before doing string matching > on the headers. > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > io/channel-websock.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > @@ -223,7 +223,7 @@ static int qio_channel_websock_handshake_process(QIOChannelWebsock *ioc, > static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc, > Error **errp) > { > - char *handshake_end; > + char *handshake_end, *tmp; > ssize_t ret; > /* Typical HTTP headers from novnc are 512 bytes, so limiting > * total header size to 4096 is easily enough. */ Drive-by grammar nit: s/easily/easy/ > @@ -249,9 +249,13 @@ static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc, > } > } > > + for (tmp = (char *)ioc->encinput.buffer; tmp < handshake_end; tmp++) { > + *tmp = g_ascii_tolower(*tmp); > + } > + > if (qio_channel_websock_handshake_process(ioc, > (char *)ioc->encinput.buffer, > - ioc->encinput.offset, > + handshake_end - (char *)ioc->encinput.buffer, I'm not sure why this change is here; nothing else in the patch changed ioc->encinput.offset.
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20170227201456.31814-1-berrange@redhat.com
Type: series
Subject: [Qemu-devel] [PATCH] io: ignore case when matching websockets HTTP headers
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
- [tag update] patchew/cover.1488220970.git.jcody@redhat.com -> patchew/cover.1488220970.git.jcody@redhat.com
Switched to a new branch 'test'
b941173 io: ignore case when matching websockets HTTP headers
=== OUTPUT BEGIN ===
Checking PATCH 1/1: io: ignore case when matching websockets HTTP headers...
ERROR: line over 90 characters
#50: FILE: io/channel-websock.c:258:
+ handshake_end - (char *)ioc->encinput.buffer,
total: 1 errors, 0 warnings, 34 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
On 02/28/2017 12:20 AM, Eric Blake wrote: > On 02/27/2017 02:14 PM, Daniel P. Berrange wrote: >> According to RFC7230 Section 3.2, header field name is case-insensitive. >> Convert the header data into all lowercase before doing string matching >> on the headers. >> >> Signed-off-by: Daniel P. Berrange <berrange@redhat.com> >> --- >> io/channel-websock.c | 14 +++++++++----- >> 1 file changed, 9 insertions(+), 5 deletions(-) >> >> @@ -223,7 +223,7 @@ static int qio_channel_websock_handshake_process(QIOChannelWebsock *ioc, >> static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc, >> Error **errp) >> { >> - char *handshake_end; >> + char *handshake_end, *tmp; >> ssize_t ret; >> /* Typical HTTP headers from novnc are 512 bytes, so limiting >> * total header size to 4096 is easily enough. */ > Drive-by grammar nit: s/easily/easy/ > >> @@ -249,9 +249,13 @@ static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc, >> } >> } >> >> + for (tmp = (char *)ioc->encinput.buffer; tmp < handshake_end; tmp++) { >> + *tmp = g_ascii_tolower(*tmp); >> + } >> + >> if (qio_channel_websock_handshake_process(ioc, >> (char *)ioc->encinput.buffer, >> - ioc->encinput.offset, >> + handshake_end - (char *)ioc->encinput.buffer, > I'm not sure why this change is here; nothing else in the patch changed > ioc->encinput.offset. > yep. The rest seems fine to me. Usage of g_ascii_strdown () seems overkill now. We do not need the header anymore after this call. Den
On Mon, Feb 27, 2017 at 03:20:37PM -0600, Eric Blake wrote: > On 02/27/2017 02:14 PM, Daniel P. Berrange wrote: > > According to RFC7230 Section 3.2, header field name is case-insensitive. > > Convert the header data into all lowercase before doing string matching > > on the headers. > > > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > > --- > > io/channel-websock.c | 14 +++++++++----- > > 1 file changed, 9 insertions(+), 5 deletions(-) > > > > > @@ -223,7 +223,7 @@ static int qio_channel_websock_handshake_process(QIOChannelWebsock *ioc, > > static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc, > > Error **errp) > > { > > - char *handshake_end; > > + char *handshake_end, *tmp; > > ssize_t ret; > > /* Typical HTTP headers from novnc are 512 bytes, so limiting > > * total header size to 4096 is easily enough. */ > > Drive-by grammar nit: s/easily/easy/ > > > @@ -249,9 +249,13 @@ static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc, > > } > > } > > > > + for (tmp = (char *)ioc->encinput.buffer; tmp < handshake_end; tmp++) { > > + *tmp = g_ascii_tolower(*tmp); > > + } > > + > > if (qio_channel_websock_handshake_process(ioc, > > (char *)ioc->encinput.buffer, > > - ioc->encinput.offset, > > + handshake_end - (char *)ioc->encinput.buffer, > > I'm not sure why this change is here; nothing else in the patch changed > ioc->encinput.offset. It is a related bug. The ioc->encinput buffer contains upto 4096 bytes, but that data may cover both the header & payload. The handshake_end pointer refers to the end of the header region. So if we just pass encinput.offset, then handshake_process() will be searching part of the payload as well as the header. This might cause it do match the wrong data. I guess I should have made that clear in the commit, or even tput it in a separate commit Regards, Daniel
On 02/28/2017 01:09 PM, Daniel P. Berrange wrote: > On Mon, Feb 27, 2017 at 03:20:37PM -0600, Eric Blake wrote: >> On 02/27/2017 02:14 PM, Daniel P. Berrange wrote: >>> According to RFC7230 Section 3.2, header field name is case-insensitive. >>> Convert the header data into all lowercase before doing string matching >>> on the headers. >>> >>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com> >>> --- >>> io/channel-websock.c | 14 +++++++++----- >>> 1 file changed, 9 insertions(+), 5 deletions(-) >>> >>> @@ -223,7 +223,7 @@ static int qio_channel_websock_handshake_process(QIOChannelWebsock *ioc, >>> static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc, >>> Error **errp) >>> { >>> - char *handshake_end; >>> + char *handshake_end, *tmp; >>> ssize_t ret; >>> /* Typical HTTP headers from novnc are 512 bytes, so limiting >>> * total header size to 4096 is easily enough. */ >> Drive-by grammar nit: s/easily/easy/ >> >>> @@ -249,9 +249,13 @@ static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc, >>> } >>> } >>> >>> + for (tmp = (char *)ioc->encinput.buffer; tmp < handshake_end; tmp++) { >>> + *tmp = g_ascii_tolower(*tmp); >>> + } >>> + >>> if (qio_channel_websock_handshake_process(ioc, >>> (char *)ioc->encinput.buffer, >>> - ioc->encinput.offset, >>> + handshake_end - (char *)ioc->encinput.buffer, >> I'm not sure why this change is here; nothing else in the patch changed >> ioc->encinput.offset. > It is a related bug. The ioc->encinput buffer contains upto 4096 > bytes, but that data may cover both the header & payload. The > handshake_end pointer refers to the end of the header region. > So if we just pass encinput.offset, then handshake_process() > will be searching part of the payload as well as the header. > This might cause it do match the wrong data. I guess I should > have made that clear in the commit, or even tput it in a > separate commit > > Regards, > Daniel with this comment this is obviously correct :) Reviewed-by: Denis V. Lunev <den@openvz.org>
On Mon, Feb 27, 2017 at 08:14:56PM +0000, Daniel P. Berrange wrote: > According to RFC7230 Section 3.2, header field name is case-insensitive. > Convert the header data into all lowercase before doing string matching > on the headers. > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > io/channel-websock.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/io/channel-websock.c b/io/channel-websock.c > index a06a4a8..32b7f37 100644 > --- a/io/channel-websock.c > +++ b/io/channel-websock.c > @@ -33,9 +33,9 @@ > #define QIO_CHANNEL_WEBSOCK_GUID "258EAFA5-E914-47DA-95CA-C5AB0DC85B11" > #define QIO_CHANNEL_WEBSOCK_GUID_LEN strlen(QIO_CHANNEL_WEBSOCK_GUID) > > -#define QIO_CHANNEL_WEBSOCK_HEADER_PROTOCOL "Sec-WebSocket-Protocol" > -#define QIO_CHANNEL_WEBSOCK_HEADER_VERSION "Sec-WebSocket-Version" > -#define QIO_CHANNEL_WEBSOCK_HEADER_KEY "Sec-WebSocket-Key" > +#define QIO_CHANNEL_WEBSOCK_HEADER_PROTOCOL "sec-websocket-protocol" > +#define QIO_CHANNEL_WEBSOCK_HEADER_VERSION "sec-websocket-version" > +#define QIO_CHANNEL_WEBSOCK_HEADER_KEY "sec-websocket-key" > > #define QIO_CHANNEL_WEBSOCK_PROTOCOL_BINARY "binary" > > @@ -223,7 +223,7 @@ static int qio_channel_websock_handshake_process(QIOChannelWebsock *ioc, > static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc, > Error **errp) > { > - char *handshake_end; > + char *handshake_end, *tmp; > ssize_t ret; > /* Typical HTTP headers from novnc are 512 bytes, so limiting > * total header size to 4096 is easily enough. */ > @@ -249,9 +249,13 @@ static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc, > } > } > > + for (tmp = (char *)ioc->encinput.buffer; tmp < handshake_end; tmp++) { > + *tmp = g_ascii_tolower(*tmp); > + } > + self-nack - this does not in fact work - while it is fine to lowercase the header keys, we must not touch the header values as some data is case-sensitive Regards, Daniel
On 02/28/2017 01:48 PM, Daniel P. Berrange wrote: > On Mon, Feb 27, 2017 at 08:14:56PM +0000, Daniel P. Berrange wrote: >> According to RFC7230 Section 3.2, header field name is case-insensitive. >> Convert the header data into all lowercase before doing string matching >> on the headers. >> >> Signed-off-by: Daniel P. Berrange <berrange@redhat.com> >> --- >> io/channel-websock.c | 14 +++++++++----- >> 1 file changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/io/channel-websock.c b/io/channel-websock.c >> index a06a4a8..32b7f37 100644 >> --- a/io/channel-websock.c >> +++ b/io/channel-websock.c >> @@ -33,9 +33,9 @@ >> #define QIO_CHANNEL_WEBSOCK_GUID "258EAFA5-E914-47DA-95CA-C5AB0DC85B11" >> #define QIO_CHANNEL_WEBSOCK_GUID_LEN strlen(QIO_CHANNEL_WEBSOCK_GUID) >> >> -#define QIO_CHANNEL_WEBSOCK_HEADER_PROTOCOL "Sec-WebSocket-Protocol" >> -#define QIO_CHANNEL_WEBSOCK_HEADER_VERSION "Sec-WebSocket-Version" >> -#define QIO_CHANNEL_WEBSOCK_HEADER_KEY "Sec-WebSocket-Key" >> +#define QIO_CHANNEL_WEBSOCK_HEADER_PROTOCOL "sec-websocket-protocol" >> +#define QIO_CHANNEL_WEBSOCK_HEADER_VERSION "sec-websocket-version" >> +#define QIO_CHANNEL_WEBSOCK_HEADER_KEY "sec-websocket-key" >> >> #define QIO_CHANNEL_WEBSOCK_PROTOCOL_BINARY "binary" >> >> @@ -223,7 +223,7 @@ static int qio_channel_websock_handshake_process(QIOChannelWebsock *ioc, >> static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc, >> Error **errp) >> { >> - char *handshake_end; >> + char *handshake_end, *tmp; >> ssize_t ret; >> /* Typical HTTP headers from novnc are 512 bytes, so limiting >> * total header size to 4096 is easily enough. */ >> @@ -249,9 +249,13 @@ static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc, >> } >> } >> >> + for (tmp = (char *)ioc->encinput.buffer; tmp < handshake_end; tmp++) { >> + *tmp = g_ascii_tolower(*tmp); >> + } >> + > self-nack - this does not in fact work - while it is fine to lowercase > the header keys, we must not touch the header values as some data is > case-sensitive > > Regards, > Daniel g-ascii-tolower() will help Den
On 02/28/2017 01:54 PM, Denis V. Lunev wrote: > On 02/28/2017 01:48 PM, Daniel P. Berrange wrote: >> On Mon, Feb 27, 2017 at 08:14:56PM +0000, Daniel P. Berrange wrote: >>> According to RFC7230 Section 3.2, header field name is case-insensitive. >>> Convert the header data into all lowercase before doing string matching >>> on the headers. >>> >>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com> >>> --- >>> io/channel-websock.c | 14 +++++++++----- >>> 1 file changed, 9 insertions(+), 5 deletions(-) >>> >>> diff --git a/io/channel-websock.c b/io/channel-websock.c >>> index a06a4a8..32b7f37 100644 >>> --- a/io/channel-websock.c >>> +++ b/io/channel-websock.c >>> @@ -33,9 +33,9 @@ >>> #define QIO_CHANNEL_WEBSOCK_GUID "258EAFA5-E914-47DA-95CA-C5AB0DC85B11" >>> #define QIO_CHANNEL_WEBSOCK_GUID_LEN strlen(QIO_CHANNEL_WEBSOCK_GUID) >>> >>> -#define QIO_CHANNEL_WEBSOCK_HEADER_PROTOCOL "Sec-WebSocket-Protocol" >>> -#define QIO_CHANNEL_WEBSOCK_HEADER_VERSION "Sec-WebSocket-Version" >>> -#define QIO_CHANNEL_WEBSOCK_HEADER_KEY "Sec-WebSocket-Key" >>> +#define QIO_CHANNEL_WEBSOCK_HEADER_PROTOCOL "sec-websocket-protocol" >>> +#define QIO_CHANNEL_WEBSOCK_HEADER_VERSION "sec-websocket-version" >>> +#define QIO_CHANNEL_WEBSOCK_HEADER_KEY "sec-websocket-key" >>> >>> #define QIO_CHANNEL_WEBSOCK_PROTOCOL_BINARY "binary" >>> >>> @@ -223,7 +223,7 @@ static int qio_channel_websock_handshake_process(QIOChannelWebsock *ioc, >>> static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc, >>> Error **errp) >>> { >>> - char *handshake_end; >>> + char *handshake_end, *tmp; >>> ssize_t ret; >>> /* Typical HTTP headers from novnc are 512 bytes, so limiting >>> * total header size to 4096 is easily enough. */ >>> @@ -249,9 +249,13 @@ static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc, >>> } >>> } >>> >>> + for (tmp = (char *)ioc->encinput.buffer; tmp < handshake_end; tmp++) { >>> + *tmp = g_ascii_tolower(*tmp); >>> + } >>> + >> self-nack - this does not in fact work - while it is fine to lowercase >> the header keys, we must not touch the header values as some data is >> case-sensitive >> >> Regards, >> Daniel > g-ascii-tolower() will help > > Den ah, sorry, wrong copy/paste. I meant 'g_ascii_strdown ()' Den
On Tue, Feb 28, 2017 at 01:58:38PM +0300, Denis V. Lunev wrote: > On 02/28/2017 01:54 PM, Denis V. Lunev wrote: > > On 02/28/2017 01:48 PM, Daniel P. Berrange wrote: > >> On Mon, Feb 27, 2017 at 08:14:56PM +0000, Daniel P. Berrange wrote: > >>> According to RFC7230 Section 3.2, header field name is case-insensitive. > >>> Convert the header data into all lowercase before doing string matching > >>> on the headers. > >>> > >>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > >>> --- > >>> io/channel-websock.c | 14 +++++++++----- > >>> 1 file changed, 9 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/io/channel-websock.c b/io/channel-websock.c > >>> index a06a4a8..32b7f37 100644 > >>> --- a/io/channel-websock.c > >>> +++ b/io/channel-websock.c > >>> @@ -33,9 +33,9 @@ > >>> #define QIO_CHANNEL_WEBSOCK_GUID "258EAFA5-E914-47DA-95CA-C5AB0DC85B11" > >>> #define QIO_CHANNEL_WEBSOCK_GUID_LEN strlen(QIO_CHANNEL_WEBSOCK_GUID) > >>> > >>> -#define QIO_CHANNEL_WEBSOCK_HEADER_PROTOCOL "Sec-WebSocket-Protocol" > >>> -#define QIO_CHANNEL_WEBSOCK_HEADER_VERSION "Sec-WebSocket-Version" > >>> -#define QIO_CHANNEL_WEBSOCK_HEADER_KEY "Sec-WebSocket-Key" > >>> +#define QIO_CHANNEL_WEBSOCK_HEADER_PROTOCOL "sec-websocket-protocol" > >>> +#define QIO_CHANNEL_WEBSOCK_HEADER_VERSION "sec-websocket-version" > >>> +#define QIO_CHANNEL_WEBSOCK_HEADER_KEY "sec-websocket-key" > >>> > >>> #define QIO_CHANNEL_WEBSOCK_PROTOCOL_BINARY "binary" > >>> > >>> @@ -223,7 +223,7 @@ static int qio_channel_websock_handshake_process(QIOChannelWebsock *ioc, > >>> static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc, > >>> Error **errp) > >>> { > >>> - char *handshake_end; > >>> + char *handshake_end, *tmp; > >>> ssize_t ret; > >>> /* Typical HTTP headers from novnc are 512 bytes, so limiting > >>> * total header size to 4096 is easily enough. */ > >>> @@ -249,9 +249,13 @@ static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc, > >>> } > >>> } > >>> > >>> + for (tmp = (char *)ioc->encinput.buffer; tmp < handshake_end; tmp++) { > >>> + *tmp = g_ascii_tolower(*tmp); > >>> + } > >>> + > >> self-nack - this does not in fact work - while it is fine to lowercase > >> the header keys, we must not touch the header values as some data is > >> case-sensitive > >> > >> Regards, > >> Daniel > > g-ascii-tolower() will help > > > > Den > ah, sorry, wrong copy/paste. I meant 'g_ascii_strdown ()' That would still lowercase both the key & value part of the headers. We need to only lowercase the key, ie text between start of line and first ':'. This requires properly parsing the HTTP header. Regards, Daniel
diff --git a/io/channel-websock.c b/io/channel-websock.c index a06a4a8..32b7f37 100644 --- a/io/channel-websock.c +++ b/io/channel-websock.c @@ -33,9 +33,9 @@ #define QIO_CHANNEL_WEBSOCK_GUID "258EAFA5-E914-47DA-95CA-C5AB0DC85B11" #define QIO_CHANNEL_WEBSOCK_GUID_LEN strlen(QIO_CHANNEL_WEBSOCK_GUID) -#define QIO_CHANNEL_WEBSOCK_HEADER_PROTOCOL "Sec-WebSocket-Protocol" -#define QIO_CHANNEL_WEBSOCK_HEADER_VERSION "Sec-WebSocket-Version" -#define QIO_CHANNEL_WEBSOCK_HEADER_KEY "Sec-WebSocket-Key" +#define QIO_CHANNEL_WEBSOCK_HEADER_PROTOCOL "sec-websocket-protocol" +#define QIO_CHANNEL_WEBSOCK_HEADER_VERSION "sec-websocket-version" +#define QIO_CHANNEL_WEBSOCK_HEADER_KEY "sec-websocket-key" #define QIO_CHANNEL_WEBSOCK_PROTOCOL_BINARY "binary" @@ -223,7 +223,7 @@ static int qio_channel_websock_handshake_process(QIOChannelWebsock *ioc, static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc, Error **errp) { - char *handshake_end; + char *handshake_end, *tmp; ssize_t ret; /* Typical HTTP headers from novnc are 512 bytes, so limiting * total header size to 4096 is easily enough. */ @@ -249,9 +249,13 @@ static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc, } } + for (tmp = (char *)ioc->encinput.buffer; tmp < handshake_end; tmp++) { + *tmp = g_ascii_tolower(*tmp); + } + if (qio_channel_websock_handshake_process(ioc, (char *)ioc->encinput.buffer, - ioc->encinput.offset, + handshake_end - (char *)ioc->encinput.buffer, errp) < 0) { return -1; }
According to RFC7230 Section 3.2, header field name is case-insensitive. Convert the header data into all lowercase before doing string matching on the headers. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- io/channel-websock.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)