Not handling case where complete payload is received but not terminating zero byte

[crdumoul]

In the Nginx error logs I'm seeing occasional error messages about bad AJP headers being received. The log line looks like this:

2011/03/31 00:49:08 [error] 10953#0: *37200 ngx_http_ajp_input_filter: bad header ajp_msg_dump(): "bad header", start:0000000001407220, pos:0000000001407220, last:0000000001407227 dump packet: 00 41 42 00 02 05 01

I've looked through the code and I believe I see the problem:

• In the function ngx_http_ajp_input_filter(), if we get to the else case at line 843 and (buf->pos + a->length == buf->last), then we have received all of the packet payload but are still missing the terminating zero byte. Note that a->length is the payload length that was encoded in the packet header. In this case a->length will become 0 as ((buf->last - buf->pos) == a->length).
• This function is using (a->length == 0) to signal a complete packet, which is incorrect, because a->length isn't accounting for the terminating zero byte. The "bad header" is because the code is expecting the 0x4142 to start a new packet but we still haven't processed the terminating zero byte from the last packet.
• I'll attach a patch which adjusts a->length to include the terminating zero byte, and adjusts the other uses of a->length accordingly.

[crdumoul]

Since I don't see any way to attach a file, I'll paste the patch here:

--- ngx_http_ajp_handler.c.orig 2011-02-12 00:44:33.000000000 -0500
+++ ngx_http_ajp_handler.c  2011-04-01 16:29:49.366773997 -0400
@@ -746,6 +746,8 @@
                     if (rc != NGX_OK) {
                         return NGX_ERROR;
                     }
+                    // account for the extra byte at the end of the chunk
+                    a->length++;

                     buf->pos += offset;
                     a->state = ngx_http_ajp_st_response_body_data_sending;
@@ -777,12 +779,13 @@

         ngx_log_debug2(NGX_LOG_DEBUG_HTTP, p->log, 0,
                 "input filter packet, length:%z, buffer_size:%z",
-                       a->length, ngx_buf_size(buf));
+                       a->length - 1, ngx_buf_size(buf));

         /* Get a zero length packet */
-        if (a->length == 0) {
+        if (a->length == 1) {
             if (buf->pos < buf->last) {
                 buf->pos++;
+                a->length--;
             }

             continue;
@@ -828,8 +831,8 @@

         /* STUB */ b->num = buf->num;

-        if (buf->pos + a->length < buf->last) {
-            buf->pos += a->length;
+        if (buf->pos + a->length <= buf->last) {
+            buf->pos += (a->length - 1);
             b->last = buf->pos;

             /* The last byte of this message always seems to be

[yaoweibin]

Thanks for this excellent bug report, Chris, you are such a good hacker.

I have added a patch for this module. But it's a little different from yours. I think the length is the meaning of ajp data packet. It should not add a one-extra byte in the length which may confuse its true meaning. So I add a new member of extra_zero_byte in the struct of ngx_http_ajp_ctx_t.

The commit is here: https://github.com/yaoweibin/nginx_ajp_module/commit/14456065de1350fd54aaf995e75db26380f8666f

I have not tested fully with this patch. Can you check out this experiment branch and test it. Thank you very much. The branch is here: https://github.com/yaoweibin/nginx_ajp_module/commits/experiment

[crdumoul]

Hi yaoweibin, I've looked at your commit, and I see one potential problem. The new member extra_zero_byte isn't initialized to zero anywhere. I think a line like:

a->extra_zero_byte = 0;

needs to be added in the function ngx_http_ajp_reinit_request().

Also, in this "if" statement:

        /* Get a zero length packet */
        if (a->length == 0 && a->extra_zero_byte && (buf->pos < buf->last)) {
            if (buf->pos < buf->last) {
                buf->pos++;
                a->extra_zero_byte = 0;
            }

            continue;
        }

the inner "if" is redundant.

[yaoweibin]

Thanks for your opinion, Chris. I think you are right. I have fixed this problem in the experiment branch.