src/usr.sbin/slowcgi: possible bug

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

src/usr.sbin/slowcgi: possible bug

temp+101
Hi tech@,

I recently checked the slowcgi(8) and found that it might have an issue
when buf_pos is at the end of buffer and buf_len is zero.

Am I right?

Index: slowcgi.c
===================================================================
RCS file: /cvs/src/usr.sbin/slowcgi/slowcgi.c,v
retrieving revision 1.50
diff -u -p -r1.50 slowcgi.c
--- slowcgi.c 4 Sep 2016 14:40:34 -0000 1.50
+++ slowcgi.c 2 Jan 2017 12:52:01 -0000
@@ -674,8 +674,8 @@ slowcgi_request(int fd, short events, vo
  /* Make space for further reads */
  if (c->buf_len > 0) {
  bcopy(c->buf + c->buf_pos, c->buf, c->buf_len);
- c->buf_pos = 0;
  }
+ c->buf_pos = 0;
  return;
 fail:
  cleanup_request(c);

Reply | Threaded
Open this post in threaded view
|

Re: src/usr.sbin/slowcgi: possible bug

temp+101
>I recently checked the slowcgi(8) and found that it might have an issue
>when buf_pos is at the end of buffer and buf_len is zero.
>
>Am I right?

It seems that all fastcgi blocks are aligned in 8-bytes and buffer size
is 8+65535+255 = 65798 bytes which is not aligned in 8-bytes. It seems
that slowcgi has no problem with aligned data, but I think in general
slowcgi should not assume that all blocks are aligned in 8-bytes.

>Index: slowcgi.c
>===================================================================
>RCS file: /cvs/src/usr.sbin/slowcgi/slowcgi.c,v
>retrieving revision 1.50
>diff -u -p -r1.50 slowcgi.c
>--- slowcgi.c 4 Sep 2016 14:40:34 -0000 1.50
>+++ slowcgi.c 2 Jan 2017 12:52:01 -0000
>@@ -674,8 +674,8 @@ slowcgi_request(int fd, short events, vo
> /* Make space for further reads */
> if (c->buf_len > 0) {
> bcopy(c->buf + c->buf_pos, c->buf, c->buf_len);
>- c->buf_pos = 0;
> }
>+ c->buf_pos = 0;
> return;
> fail:
> cleanup_request(c);
>
>

Reply | Threaded
Open this post in threaded view
|

Re: src/usr.sbin/slowcgi: possible bug

Florian Obser-2
In reply to this post by temp+101
On Mon, Jan 02, 2017 at 04:29:21PM +0330, [hidden email] wrote:
> Hi tech@,
>
> I recently checked the slowcgi(8) and found that it might have an issue
> when buf_pos is at the end of buffer and buf_len is zero.
>
> Am I right?

we can simplify this even more. There is no need to remember the
buffer position outside of this function. It will be 0 on every call,
either because we made progress in parsing data and then copied the
rest to the beginning of the buffer or we did not make progress at
all, and then we need to start parsing from the beginning again.

OK?

diff --git slowcgi.c slowcgi.c
index dec4df8d1a1..83d12d99160 100644
--- slowcgi.c
+++ slowcgi.c
@@ -118,7 +118,6 @@ struct request {
  struct event tmo;
  int fd;
  uint8_t buf[FCGI_RECORD_SIZE];
- size_t buf_pos;
  size_t buf_len;
  struct fcgi_response_head response_head;
  struct fcgi_stdin_head stdin_head;
@@ -495,7 +494,6 @@ slowcgi_accept(int fd, short events, void *arg)
  return;
  }
  c->fd = s;
- c->buf_pos = 0;
  c->buf_len = 0;
  c->request_started = 0;
  c->stdin_fd_closed = c->stdout_fd_closed = c->stderr_fd_closed = 0;
@@ -632,12 +630,12 @@ slowcgi_request(int fd, short events, void *arg)
 {
  struct request *c;
  ssize_t n;
- size_t parsed;
+ size_t parsed, pos = 0;
 
  c = arg;
 
- n = read(fd, c->buf + c->buf_pos + c->buf_len,
-    FCGI_RECORD_SIZE - c->buf_pos-c->buf_len);
+ n = read(fd, c->buf + c->buf_len,
+    FCGI_RECORD_SIZE - c->buf_len);
 
  switch (n) {
  case -1:
@@ -666,16 +664,15 @@ slowcgi_request(int fd, short events, void *arg)
  * at that point, which is what happens here.
  */
  do {
- parsed = parse_record(c->buf + c->buf_pos, c->buf_len, c);
- c->buf_pos += parsed;
+ parsed = parse_record(c->buf + pos, c->buf_len, c);
+ pos += parsed;
  c->buf_len -= parsed;
  } while (parsed > 0 && c->buf_len > 0);
 
  /* Make space for further reads */
- if (c->buf_len > 0) {
- bcopy(c->buf + c->buf_pos, c->buf, c->buf_len);
- c->buf_pos = 0;
- }
+ if (c->buf_len > 0 && pos > 0)
+ bcopy(c->buf + pos, c->buf, c->buf_len);
+
  return;
 fail:
  cleanup_request(c);

Reply | Threaded
Open this post in threaded view
|

Re: src/usr.sbin/slowcgi: possible bug

temp+101
In reply to this post by temp+101
Florian Obser wrotes:

>On Mon, Jan 02, 2017 at 04:29:21PM +0330, [hidden email] wrote:
>> Hi tech@,
>>
>> I recently checked the slowcgi(8) and found that it might have an issue
>> when buf_pos is at the end of buffer and buf_len is zero.
>>
>> Am I right?
>
>we can simplify this even more. There is no need to remember the
>buffer position outside of this function. It will be 0 on every call,
>either because we made progress in parsing data and then copied the
>rest to the beginning of the buffer or we did not make progress at
>all, and then we need to start parsing from the beginning again.
>
>OK?

It seems perfect.

>
>diff --git slowcgi.c slowcgi.c
>index dec4df8d1a1..83d12d99160 100644
>--- slowcgi.c
>+++ slowcgi.c
>@@ -118,7 +118,6 @@ struct request {
> struct event tmo;
> int fd;
> uint8_t buf[FCGI_RECORD_SIZE];
>- size_t buf_pos;
> size_t buf_len;
> struct fcgi_response_head response_head;
> struct fcgi_stdin_head stdin_head;
>@@ -495,7 +494,6 @@ slowcgi_accept(int fd, short events, void *arg)
> return;
> }
> c->fd = s;
>- c->buf_pos = 0;
> c->buf_len = 0;
> c->request_started = 0;
> c->stdin_fd_closed = c->stdout_fd_closed = c->stderr_fd_closed = 0;
>@@ -632,12 +630,12 @@ slowcgi_request(int fd, short events, void *arg)
> {
> struct request *c;
> ssize_t n;
>- size_t parsed;
>+ size_t parsed, pos = 0;
>
> c = arg;
>
>- n = read(fd, c->buf + c->buf_pos + c->buf_len,
>-    FCGI_RECORD_SIZE - c->buf_pos-c->buf_len);
>+ n = read(fd, c->buf + c->buf_len,
>+    FCGI_RECORD_SIZE - c->buf_len);
>
> switch (n) {
> case -1:
>@@ -666,16 +664,15 @@ slowcgi_request(int fd, short events, void *arg)
> * at that point, which is what happens here.
> */
> do {
>- parsed = parse_record(c->buf + c->buf_pos, c->buf_len, c);
>- c->buf_pos += parsed;
>+ parsed = parse_record(c->buf + pos, c->buf_len, c);
>+ pos += parsed;
> c->buf_len -= parsed;
> } while (parsed > 0 && c->buf_len > 0);
>
> /* Make space for further reads */
>- if (c->buf_len > 0) {
>- bcopy(c->buf + c->buf_pos, c->buf, c->buf_len);
>- c->buf_pos = 0;
>- }
>+ if (c->buf_len > 0 && pos > 0)
>+ bcopy(c->buf + pos, c->buf, c->buf_len);
>+
> return;
> fail:
> cleanup_request(c);
>
>