You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Mark Drayton <ma...@markdrayton.info> on 2007/08/24 17:32:32 UTC
2.0.59: ETag mtimes on 32- and 64-bit machines
Hi there
Forgive me if this is the wrong list. It's not really a user question but
I'm not sure it's a dev question, either, because I'm just looking for
clarification that my changes are correct.
We have a mix of 32- and 64-bit machines in our server farm across which
we'd like to guarantee consistent ETags (inodes turned off, of course). As
discussed here:
http://issues.apache.org/bugzilla/show_bug.cgi?id=40064
the ETags differ between architectures:
[draytm01@dev draytm01]$ GET -ed http://32bit/images/test.jpg | egrep
'(ETag|Last-M|Length)'
ETag: "2e30-9b91cfc0"
Content-Length: 11824
Last-Modified: Fri, 17 Sep 2004 10:27:19 GMT
[draytm01@dev draytm01]$ GET -ed http://64bit/images/test.jpg | egrep
'(ETag|Last-M|Length)'
ETag: "2e30-3e4469b91cfc0"
Content-Length: 11824
Last-Modified: Fri, 17 Sep 2004 10:27:19 GMT
The problem is that the 64-bit (apr_uint64_t) mtime is cast to an unsigned
long before conversion to hex, effectively wiping out the high 32 bits of
the mtime on a 32-bit machine.
Issue #40064 has a patch for Apache 2.2 which changes etag_ulong_to_hex() to
etag_uint64_to_hex() and avoids casting the mtime to an (arch-dependent)
unsigned long. We can't move to 2.2 at the moment so instead I patched
2.0.59 with the same changes (diff below -- note 2.2.x moved this code out
to http_etag.c). Initially it didn't work -- the 32-bit machine still
returned a truncated ETag. I fixed it with (in etag_uint64_to_hex()):
- int shift = sizeof(unsigned long) * 8 - 4;
+ int shift = sizeof(apr_uint64_t) * 8 - 4;
Is this right? I'm not a C programmer but it seems right to me: without this
change etag_uint64_to_hex() only converts the low 32 bits (ie, length of an
unsigned int on a 32-bit machine). So now I have:
[draytm01@dev draytm01]$ GET -ed http://32bit/images/test.jpg | egrep
'(ETag|Last-M|Length)'
ETag: "2e30-3e4469b91cfc0"
Content-Length: 11824
Last-Modified: Fri, 17 Sep 2004 10:27:19 GMT
[draytm01@dev draytm01]$ GET -ed http://64bit/images/test.jpg | egrep
'(ETag|Last-M|Length)'
ETag: "2e30-3e4469b91cfc0"
Content-Length: 11824
Last-Modified: Fri, 17 Sep 2004 10:27:19 GMT
If this is the correct fix then perhaps it should be applied to 2.2.x. If
it's not, perhaps you could point me in the right direction :~) I'm guessing
there won't be any interest in incorporating it into 2.0.x as 32-bit users
will suddenly see their ETags change between point releases.
Looking forward to any comments,
Mark Drayton
diff -ur httpd-2.0.59-orig/modules/http/http_protocol.c httpd-2.0.59
/modules/http/http_protocol.c
--- httpd-2.0.59-orig/modules/http/http_protocol.c 2006-07-12 08:40:
55.000000000 +0100
+++ httpd-2.0.59/modules/http/http_protocol.c 2007-08-24 16:06:
56.000000000 +0100
@@ -2698,16 +2698,17 @@
l->method_list->nelts = 0;
}
-/* Generate the human-readable hex representation of an unsigned long
+/* Generate the human-readable hex representation of an apr_uin64_t
* (basically a faster version of 'sprintf("%lx")')
+ * (basically a faster version of 'sprintf("%llx")')
*/
#define HEX_DIGITS "0123456789abcdef"
-static char *etag_ulong_to_hex(char *next, unsigned long u)
+static char *etag_uint64_to_hex(char *next, apr_uint64_t u)
{
int printing = 0;
int shift = sizeof(unsigned long) * 8 - 4;
do {
- unsigned long next_digit = ((u >> shift) & (unsigned long)0xf);
+ unsigned short next_digit = ((u >> shift) & (apr_uint64_t)0xf);
if (next_digit) {
*next++ = HEX_DIGITS[next_digit];
printing = 1;
@@ -2717,12 +2718,12 @@
}
shift -= 4;
} while (shift);
- *next++ = HEX_DIGITS[u & (unsigned long)0xf];
+ *next++ = HEX_DIGITS[u & (apr_uint64_t)0xf];
return next;
}
#define ETAG_WEAK "W/"
-#define CHARS_PER_UNSIGNED_LONG (sizeof(unsigned long) * 2)
+#define CHARS_PER_UINT64 (sizeof(apr_uint64_t) * 2)
/*
* Construct an entity tag (ETag) from resource information. If it's a
real
* file, build in some of the file characteristics. If the modification
time
@@ -2785,7 +2786,7 @@
* FileETag keywords.
*/
etag = apr_palloc(r->pool, weak_len + sizeof("\"--\"") +
- 3 * CHARS_PER_UNSIGNED_LONG + 1);
+ 3 * CHARS_PER_UINT64 + 1);
next = etag;
if (weak) {
while (*weak) {
@@ -2795,21 +2796,21 @@
*next++ = '"';
bits_added = 0;
if (etag_bits & ETAG_INODE) {
- next = etag_ulong_to_hex(next, (unsigned long)r->finfo.inode);
+ next = etag_uint64_to_hex(next, r->finfo.inode);
bits_added |= ETAG_INODE;
}
if (etag_bits & ETAG_SIZE) {
if (bits_added != 0) {
*next++ = '-';
}
- next = etag_ulong_to_hex(next, (unsigned long)r->finfo.size);
+ next = etag_uint64_to_hex(next, r->finfo.size);
bits_added |= ETAG_SIZE;
}
if (etag_bits & ETAG_MTIME) {
if (bits_added != 0) {
*next++ = '-';
}
- next = etag_ulong_to_hex(next, (unsigned long)r->mtime);
+ next = etag_uint64_to_hex(next, r->mtime);
}
*next++ = '"';
*next = '\0';
@@ -2819,7 +2820,7 @@
* Not a file document, so just use the mtime: [W/]"mtime"
*/
etag = apr_palloc(r->pool, weak_len + sizeof("\"\"") +
- CHARS_PER_UNSIGNED_LONG + 1);
+ CHARS_PER_UINT64 + 1);
next = etag;
if (weak) {
while (*weak) {
@@ -2827,7 +2828,7 @@
}
}
*next++ = '"';
- next = etag_ulong_to_hex(next, (unsigned long)r->mtime);
+ next = etag_uint64_to_hex(next, r->mtime);
*next++ = '"';
*next = '\0';
}
Re: 2.0.59: ETag mtimes on 32- and 64-bit machines
Posted by Joe Orton <jo...@redhat.com>.
On Fri, Aug 24, 2007 at 04:32:32PM +0100, Mark Drayton wrote:
...
> Issue #40064 has a patch for Apache 2.2 which changes etag_ulong_to_hex() to
> etag_uint64_to_hex() and avoids casting the mtime to an (arch-dependent)
> unsigned long. We can't move to 2.2 at the moment so instead I patched
> 2.0.59 with the same changes (diff below -- note 2.2.x moved this code out
> to http_etag.c). Initially it didn't work -- the 32-bit machine still
> returned a truncated ETag. I fixed it with (in etag_uint64_to_hex()):
>
> - int shift = sizeof(unsigned long) * 8 - 4;
> + int shift = sizeof(apr_uint64_t) * 8 - 4;
>
> Is this right? I'm not a C programmer but it seems right to me: without this
> change etag_uint64_to_hex() only converts the low 32 bits (ie, length of an
> unsigned int on a 32-bit machine). So now I have:
Yes, that's correct - this was fixed in Subversion exactly as you
describe:
http://svn.apache.org/viewvc?view=rev&rev=517238 (initial patch)
http://svn.apache.org/viewvc?view=rev&rev=517654 (corrected)
I've added a note to this effect to the bug report.
joe