You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jim Jagielski <ji...@jaguNET.com> on 2006/01/08 19:26:05 UTC
Re: svn commit: r366926 - in /httpd/httpd/branches/fcgi-proxy-dev/modules/proxy:
rooneg@apache.org wrote:
>
> Author: rooneg
> Date: Sat Jan 7 13:37:40 2006
> New Revision: 366926
>
Weird... Just yesterday I did the below, which allows us to
keep using FCGI headers where "natural" yet also resolves the
struct stuff.... I think the below is "simpler" since it
isolates things a bit.
Index: modules/proxy/fcgi_protocol.h
===================================================================
--- modules/proxy/fcgi_protocol.h (revision 366724)
+++ modules/proxy/fcgi_protocol.h (working copy)
@@ -77,6 +77,7 @@
unsigned char reserved[5];
} fcgi_begin_request_body;
+typedef unsigned char fcgi_array;
#endif /* FCGI_PROTOCOL_H */
/** @} */
Index: modules/proxy/mod_proxy_fcgi.c
===================================================================
--- modules/proxy/mod_proxy_fcgi.c (revision 366724)
+++ modules/proxy/mod_proxy_fcgi.c (working copy)
@@ -20,6 +20,47 @@
module AP_MODULE_DECLARE_DATA proxy_fcgi_module;
/*
+ * The below 3 functions serve to map the FCGI structs
+ * back and forth between an 8 byte array. We do this to avoid
+ * any potential padding issues when we send or read these
+ * structures
+ */
+static void fcgi_header_to_array(fcgi_header *h, fcgi_array a[])
+{
+ a[0] = h->version;
+ a[1] = h->type;
+ a[2] = h->requestIdB1;
+ a[3] = h->requestIdB0;
+ a[4] = h->contentLengthB1;
+ a[5] = h->contentLengthB0;
+ a[6] = h->paddingLength;
+ a[7] = h->reserved;
+}
+
+static void fcgi_header_from_array(fcgi_header *h, fcgi_array a[])
+{
+ h->version = a[0];
+ h->type = a[1];
+ h->requestIdB1 = a[2];
+ h->requestIdB0 = a[3];
+ h->contentLengthB1 = a[4];
+ h->contentLengthB0 = a[5];
+ h->paddingLength = a[6];
+ h->reserved = a[7];
+}
+
+static void fcgi_begin_request_body_to_array(fcgi_begin_request_body *h, fcgi_array a[])
+{
+ a[0] = h->roleB1;
+ a[1] = h->roleB0;
+ a[2] = h->flags;
+ a[3] = h->reserved[0];
+ a[4] = h->reserved[1];
+ a[5] = h->reserved[2];
+ a[6] = h->reserved[3];
+ a[7] = h->reserved[4];
+}
+/*
* Canonicalise http-like URLs.
* scheme is the scheme for the URL
* url is the URL starting with the first '/'
@@ -105,7 +146,11 @@
{
struct iovec vec[2];
fcgi_header header;
+ fcgi_array farray[FCGI_HEADER_LEN];
+
fcgi_begin_request_body brb;
+ fcgi_array abrb[FCGI_HEADER_LEN];
+
apr_size_t len;
fill_in_header(&header, FCGI_BEGIN_REQUEST, request_id);
@@ -117,10 +162,13 @@
brb.roleB0 = ((FCGI_RESPONDER) & 0xff);
brb.flags = FCGI_KEEP_CONN;
- vec[0].iov_base = &header;
- vec[0].iov_len = sizeof(header);
- vec[1].iov_base = &brb;
- vec[1].iov_len = sizeof(brb);
+ fcgi_header_to_array(&header, farray);
+ fcgi_begin_request_body_to_array(&brb, abrb);
+
+ vec[0].iov_base = farray;
+ vec[0].iov_len = sizeof(farray);
+ vec[1].iov_base = abrb;
+ vec[1].iov_len = sizeof(abrb);
return apr_socket_sendv(conn->sock, vec, 2, &len);
}
@@ -132,6 +180,7 @@
const apr_table_entry_t *elts;
struct iovec vec[2];
fcgi_header header;
+ fcgi_array farray[FCGI_HEADER_LEN];
apr_size_t bodylen;
char *body, *itr;
apr_status_t rv;
@@ -239,8 +288,10 @@
header.contentLengthB0 = ((bodylen) & 0xff);
header.paddingLength = 0;
- vec[0].iov_base = &header;
- vec[0].iov_len = sizeof(header);
+ fcgi_header_to_array(&header, farray);
+
+ vec[0].iov_base = farray;
+ vec[0].iov_len = sizeof(farray);
vec[1].iov_base = body;
vec[1].iov_len = bodylen;
@@ -254,9 +305,11 @@
header.contentLengthB0 = 0;
header.paddingLength = 0;
- vec[0].iov_base = &header;
- vec[0].iov_len = sizeof(header);
+ fcgi_header_to_array(&header, farray);
+ vec[0].iov_base = farray;
+ vec[0].iov_len = sizeof(farray);
+
return apr_socket_sendv(conn->sock, vec, 1, &len);
}
@@ -381,6 +434,8 @@
conn_rec *c = r->connection;
struct iovec vec[2];
fcgi_header header;
+ fcgi_array farray[FCGI_HEADER_LEN];
+
apr_pollfd_t pfd;
fill_in_header(&header, FCGI_STDIN, request_id);
@@ -431,8 +486,10 @@
header.contentLengthB1 = ((writebuflen >> 8) & 0xff);
header.contentLengthB0 = ((writebuflen) & 0xff);
- vec[0].iov_base = &header;
- vec[0].iov_len = sizeof(header);
+ fcgi_header_to_array(&header, farray);
+
+ vec[0].iov_base = farray;
+ vec[0].iov_len = sizeof(farray);
vec[1].iov_base = writebuf;
vec[1].iov_len = writebuflen;
@@ -453,9 +510,11 @@
header.contentLengthB1 = 0;
header.contentLengthB0 = 0;
- vec[0].iov_base = &header;
- vec[0].iov_len = sizeof(header);
+ fcgi_header_to_array(&header, farray);
+ vec[0].iov_base = farray;
+ vec[0].iov_len = sizeof(farray);
+
rv = apr_socket_sendv(conn->sock, vec, 1, &len);
}
}
@@ -470,26 +529,22 @@
int rid, type;
apr_bucket *b;
char plen;
- /*
- * below mapped to fcgi_header layout. We
- * use a unsigned char array to ensure the
- * shifts are correct and avoid any potential
- * internal padding when using structs.
- */
- unsigned char fheader[FCGI_HEADER_LEN];
+ fcgi_array farray[FCGI_HEADER_LEN];
+ fcgi_header fheader;
- memset(fheader, 0, FCGI_HEADER_LEN);
+
+ memset(farray, 0, FCGI_HEADER_LEN);
memset(readbuf, 0, sizeof(readbuf));
/* First, we grab the header... */
readbuflen = FCGI_HEADER_LEN;
- rv = apr_socket_recv(conn->sock, (char *) fheader, &readbuflen);
+ rv = apr_socket_recv(conn->sock, (char *) farray, &readbuflen);
if (rv != APR_SUCCESS) {
break;
}
- dump_header_to_log(r, fheader, readbuflen);
+ dump_header_to_log(r, farray, readbuflen);
if (readbuflen != FCGI_HEADER_LEN) {
ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
@@ -500,17 +555,19 @@
break;
}
- if (fheader[0] != FCGI_VERSION) {
+ fcgi_header_from_array(&fheader, farray);
+
+ if (fheader.version != FCGI_VERSION) {
ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
"proxy: FCGI: Got bogus version %d",
- (int) fheader[0]);
+ (int) fheader.version);
rv = APR_EINVAL;
break;
}
- type = fheader[1];
+ type = fheader.type;
- rid = (fheader[2] << 8) | fheader[3];
+ rid = (fheader.requestIdB1 << 8) | fheader.requestIdB0;
if (rid != request_id) {
ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
@@ -522,9 +579,9 @@
#endif
}
- clen = (fheader[4] << 8) | fheader[5];
+ clen = (fheader.contentLengthB1 << 8) | fheader.contentLengthB0;
- plen = fheader[6];
+ plen = fheader.paddingLength;
recv_again:
if (clen > sizeof(readbuf) - 1) {
--
===========================================================================
Jim Jagielski [|] jim@jaguNET.com [|] http://www.jaguNET.com/
"If you can dodge a wrench, you can dodge a ball."
Re: svn commit: r366926 - in /httpd/httpd/branches/fcgi-proxy-dev/modules/proxy:
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 1/8/06, Jim Jagielski <ji...@jagunet.com> wrote:
>
> On Jan 8, 2006, at 1:46 PM, Garrett Rooney wrote:
>
> > On 1/8/06, Jim Jagielski <ji...@jagunet.com> wrote:
> >> rooneg@apache.org wrote:
> >>>
> >>> Author: rooneg
> >>> Date: Sat Jan 7 13:37:40 2006
> >>> New Revision: 366926
> >>>
> >>
> >> Weird... Just yesterday I did the below, which allows us to
> >> keep using FCGI headers where "natural" yet also resolves the
> >> struct stuff.... I think the below is "simpler" since it
> >> isolates things a bit.
> >
>
> Mine also corrects the use of the 'brb' struct.
>
Yeah, I was going to deal with that one, but hadn't gotten around to it yet ;-)
-garrett
Re: svn commit: r366926 - in /httpd/httpd/branches/fcgi-proxy-dev/modules/proxy:
Posted by Jim Jagielski <ji...@jagunet.com>.
On Jan 8, 2006, at 1:46 PM, Garrett Rooney wrote:
> On 1/8/06, Jim Jagielski <ji...@jagunet.com> wrote:
>> rooneg@apache.org wrote:
>>>
>>> Author: rooneg
>>> Date: Sat Jan 7 13:37:40 2006
>>> New Revision: 366926
>>>
>>
>> Weird... Just yesterday I did the below, which allows us to
>> keep using FCGI headers where "natural" yet also resolves the
>> struct stuff.... I think the below is "simpler" since it
>> isolates things a bit.
>
Mine also corrects the use of the 'brb' struct.
Re: svn commit: r366926 - in /httpd/httpd/branches/fcgi-proxy-dev/modules/proxy:
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 1/8/06, Jim Jagielski <ji...@jagunet.com> wrote:
> If it's OK, I'll merge the best aspects of both together and commit
> that...
> Basically, it would be abstracting out the mapping between the header
> struct and the actual array used, to use the header when logical
> but avoid the mess of loads of array offset defines in the actual
> code flow. Most likely tomorrow :)
>
Works for me. Like I said, I don't have any overly strong feelings on
the approach, as long as we're not writing actual structures over the
wire and we avoid duplicated logic about the fastcgi protocol I'm
happy.
-garrett
Re: svn commit: r366926 - in /httpd/httpd/branches/fcgi-proxy-dev/modules/proxy:
Posted by Jim Jagielski <ji...@jagunet.com>.
If it's OK, I'll merge the best aspects of both together and commit
that...
Basically, it would be abstracting out the mapping between the header
struct and the actual array used, to use the header when logical
but avoid the mess of loads of array offset defines in the actual
code flow. Most likely tomorrow :)
Re: svn commit: r366926 - in /httpd/httpd/branches/fcgi-proxy-dev/modules/proxy:
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 1/8/06, Jim Jagielski <ji...@jagunet.com> wrote:
> rooneg@apache.org wrote:
> >
> > Author: rooneg
> > Date: Sat Jan 7 13:37:40 2006
> > New Revision: 366926
> >
>
> Weird... Just yesterday I did the below, which allows us to
> keep using FCGI headers where "natural" yet also resolves the
> struct stuff.... I think the below is "simpler" since it
> isolates things a bit.
One thing I prefer about the way I did it is that the code for taking
a content length and splitting it up into the two bytes goes in a
single place, rather showing up in a few different places.
I'm also not sure I like the fcgi_array typedef. It would be one
thing if it was able to ensure that the array passed around was 8
bytes long, but as it stands it just seems to hide the fact that it's
an array of unsigned char's.
-garrett