Add padding to PUSH_PROMISE

lib/nghttp2_frame.c

@@ -266,7 +266,7 @@ ssize_t nghttp2_frame_pack_headers(uint8_t **buf_ptr,
   if(frame->hd.flags & NGHTTP2_FLAG_PRIORITY) {
     nghttp2_put_uint32be(&(*buf_ptr)[payloadoff], frame->pri);
   }
-  return frame->hd.length + NGHTTP2_FRAME_HEAD_LENGTH + *bufoff_ptr;
+  return *bufoff_ptr + NGHTTP2_FRAME_HEAD_LENGTH + frame->hd.length;
 }
 
 int nghttp2_frame_unpack_headers_payload(nghttp2_headers *frame,
@@ -396,35 +396,45 @@ int nghttp2_frame_unpack_settings_payload2(nghttp2_settings_entry **iv_ptr,
 
 ssize_t nghttp2_frame_pack_push_promise(uint8_t **buf_ptr,
                                         size_t *buflen_ptr,
+                                        size_t *bufoff_ptr,
                                         nghttp2_push_promise *frame,
                                         nghttp2_hd_deflater *deflater)
 {
-  ssize_t framelen;
+  size_t payloadoff = NGHTTP2_FRAME_HEAD_LENGTH + 2;
-  size_t nv_offset = NGHTTP2_FRAME_HEAD_LENGTH + 4;
+  size_t nv_offset = payloadoff + 4;
   ssize_t rv;
+  size_t payloadlen;
+
   rv = nghttp2_hd_deflate_hd(deflater, buf_ptr, buflen_ptr, nv_offset,
                              frame->nva, frame->nvlen);
   if(rv < 0) {
     return rv;
   }
-  framelen = rv + nv_offset;
+
-  if(NGHTTP2_FRAME_HEAD_LENGTH + NGHTTP2_MAX_FRAME_LENGTH < rv + nv_offset) {
+  payloadlen = 4 + rv;
-    frame->hd.length = NGHTTP2_MAX_FRAME_LENGTH;
+
-    frame->hd.flags &= ~NGHTTP2_FLAG_END_HEADERS;
+  *bufoff_ptr = 2;
-  } else {
+  frame->padlen = 0;
-    frame->hd.length = framelen - NGHTTP2_FRAME_HEAD_LENGTH;
+  frame->hd.length = payloadlen;
-  }
   /* If frame->nvlen == 0, *buflen_ptr may be smaller than
      nv_offset */
   rv = nghttp2_reserve_buffer(buf_ptr, buflen_ptr, nv_offset);
   if(rv < 0) {
     return rv;
   }
-  memset(*buf_ptr, 0, nv_offset);
+  memset(*buf_ptr + *bufoff_ptr, 0, NGHTTP2_FRAME_HEAD_LENGTH);
   /* pack ctrl header after length is determined */
-  nghttp2_frame_pack_frame_hd(*buf_ptr, &frame->hd);
+  if(NGHTTP2_MAX_FRAME_LENGTH < payloadlen) {
-  nghttp2_put_uint32be(&(*buf_ptr)[8], frame->promised_stream_id);
+    /* Needs CONTINUATION */
-  return framelen;
+    nghttp2_frame_hd hd = frame->hd;
+    hd.flags &= ~NGHTTP2_FLAG_END_HEADERS;
+    hd.length = NGHTTP2_MAX_FRAME_LENGTH;
+    nghttp2_frame_pack_frame_hd(*buf_ptr + *bufoff_ptr, &hd);
+  } else {
+    nghttp2_frame_pack_frame_hd(*buf_ptr + *bufoff_ptr, &frame->hd);
+  }
+  nghttp2_put_uint32be(&(*buf_ptr)[payloadoff], frame->promised_stream_id);
+  return *bufoff_ptr + NGHTTP2_FRAME_HEAD_LENGTH + frame->hd.length;
 }
 
 int nghttp2_frame_unpack_push_promise_payload(nghttp2_push_promise *frame,

lib/nghttp2_frame.h

@@ -256,12 +256,20 @@ int nghttp2_frame_unpack_settings_payload2(nghttp2_settings_entry **iv_ptr,
  * expansion occurred, memory previously pointed by |*buf_ptr| may
  * change.  |*buf_ptr| and |*buflen_ptr| are updated accordingly.
  *
+ * The first byte the frame is serialized is returned in the
+ * |*bufoff_ptr|. Currently, it is always 2 to account for possible
+ * PAD_HIGH and PAD_LOW.
+ *
  * frame->hd.length is assigned after length is determined during
  * packing process. If payload length is strictly larger than
  * NGHTTP2_MAX_FRAME_LENGTH, payload data is still serialized as is,
  * but frame->hd.length is set to NGHTTP2_MAX_FRAME_LENGTH and
  * NGHTTP2_FLAG_END_HEADERS flag is cleared from frame->hd.flags.
  *
+ * This function returns the size of packed frame (which includes
+ * |*bufoff_ptr| bytes) if it succeeds, or returns one of the
+ * following negative error codes:
+ *
  * This function returns the size of packed frame if it succeeds, or
  * returns one of the following negative error codes:
  *
@@ -274,6 +282,7 @@ int nghttp2_frame_unpack_settings_payload2(nghttp2_settings_entry **iv_ptr,
  */
 ssize_t nghttp2_frame_pack_push_promise(uint8_t **buf_ptr,
                                         size_t *buflen_ptr,
+                                        size_t *bufoff_ptr,
                                         nghttp2_push_promise *frame,
                                         nghttp2_hd_deflater *deflater);
 

lib/nghttp2_session.c

@@ -1095,6 +1095,76 @@ static ssize_t session_call_select_padding(nghttp2_session *session,
   return frame->hd.length;
 }
 
+/* Add padding to HEADERS or PUSH_PROMISE. We use
+   frame->headers.padlen in this function to use the fact that
+   frame->push_promise has also padlen in the same position. */
+static ssize_t session_headers_add_pad(nghttp2_session *session,
+                                       nghttp2_frame *frame)
+{
+  int rv;
+  ssize_t padded_payloadlen;
+
+  padded_payloadlen = session_call_select_padding(session, frame,
+                                                  frame->hd.length + 1024);
+  if(nghttp2_is_fatal(padded_payloadlen)) {
+    return padded_payloadlen;
+  }
+
+  frame->headers.padlen = padded_payloadlen - frame->hd.length;
+  frame->hd.length = padded_payloadlen;
+
+  DEBUGF(fprintf(stderr, "payloadlen=%zu, padlen=%zu\n",
+                 frame->hd.length, frame->headers.padlen));
+
+  if(frame->hd.length > NGHTTP2_MAX_FRAME_LENGTH) {
+    nghttp2_frame_hd hd = frame->hd;
+    hd.flags &= ~NGHTTP2_FLAG_END_HEADERS;
+    hd.length = NGHTTP2_MAX_FRAME_LENGTH;
+
+    if(NGHTTP2_MAX_FRAME_LENGTH >
+       frame->hd.length - frame->headers.padlen) {
+      size_t padlen = NGHTTP2_MAX_FRAME_LENGTH -
+        (frame->hd.length - frame->headers.padlen);
+
+      DEBUGF(fprintf(stderr, "padding across 2 frames\n"));
+      DEBUGF(fprintf(stderr, "first HEADERS/PUSH_PROMISE "
+                     "payloadlen=%zu, padlen=%zu\n", hd.length, padlen));
+
+      rv = nghttp2_frame_add_pad(&session->aob.framebuf,
+                                 &session->aob.framebufmax,
+                                 &session->aob.framebufoff,
+                                 &hd.flags,
+                                 hd.length - padlen,
+                                 padlen);
+      if(nghttp2_is_fatal(rv)) {
+        return rv;
+      }
+    } else {
+      /* PAD_HIGH and PAD_LOW will be added in
+         nghttp2_session_after_frame_sent(). */
+    }
+    nghttp2_frame_pack_frame_hd
+      (session->aob.framebuf + session->aob.framebufoff, &hd);
+    /* At this point, framebuflen > session->aob.framebufmax. But
+       before we access the missing part, we will allocate it in
+       nghttp2_session_after_frame_sent(). */
+  } else if(frame->headers.padlen > 0) {
+    rv = nghttp2_frame_add_pad(&session->aob.framebuf,
+                               &session->aob.framebufmax,
+                               &session->aob.framebufoff,
+                               &frame->hd.flags,
+                               frame->hd.length - frame->headers.padlen,
+                               frame->headers.padlen);
+    if(nghttp2_is_fatal(rv)) {
+      return rv;
+    }
+    nghttp2_frame_pack_frame_hd
+      (session->aob.framebuf + session->aob.framebufoff, &frame->hd);
+  }
+  return session->aob.framebufoff + NGHTTP2_FRAME_HEAD_LENGTH
+    + frame->hd.length;
+}
+
 static ssize_t nghttp2_session_prep_frame(nghttp2_session *session,
                                           nghttp2_outbound_item *item)
 {
@@ -1106,7 +1176,6 @@ static ssize_t nghttp2_session_prep_frame(nghttp2_session *session,
     case NGHTTP2_HEADERS: {
       int r;
       nghttp2_headers_aux_data *aux_data;
-      ssize_t padded_payloadlen;
       aux_data = (nghttp2_headers_aux_data*)item->aux_data;
       if(frame->hd.stream_id == -1) {
         /* initial HEADERS, which opens stream */
@@ -1140,71 +1209,9 @@ static ssize_t nghttp2_session_prep_frame(nghttp2_session *session,
       if(framebuflen < 0) {
         return framebuflen;
       }
-      padded_payloadlen = session_call_select_padding
+      framebuflen = session_headers_add_pad(session, frame);
-        (session, frame, frame->hd.length + 1024);
+      if(framebuflen < 0) {
-      if(nghttp2_is_fatal(padded_payloadlen)) {
+        return framebuflen;
-        return padded_payloadlen;
-      }
-
-      frame->headers.padlen = padded_payloadlen - frame->hd.length;
-      frame->hd.length = padded_payloadlen;
-
-      DEBUGF(fprintf(stderr, "payloadlen=%zu, padlen=%zu\n",
-                     frame->hd.length, frame->headers.padlen));
-
-      if(frame->hd.length > NGHTTP2_MAX_FRAME_LENGTH) {
-        if(NGHTTP2_MAX_FRAME_LENGTH >
-           frame->hd.length - frame->headers.padlen) {
-          size_t padlen = NGHTTP2_MAX_FRAME_LENGTH -
-            (frame->hd.length - frame->headers.padlen);
-          nghttp2_frame_hd hd = frame->hd;
-
-          DEBUGF(fprintf(stderr, "padding across 2 frames\n"));
-
-          hd.flags &= ~NGHTTP2_FLAG_END_HEADERS;
-          hd.length = NGHTTP2_MAX_FRAME_LENGTH;
-
-          DEBUGF(fprintf(stderr, "first HEADERS payloadlen=%zu, padlen=%zu\n",
-                         hd.length, padlen));
-
-          r = nghttp2_frame_add_pad(&session->aob.framebuf,
-                                    &session->aob.framebufmax,
-                                    &session->aob.framebufoff,
-                                    &hd.flags,
-                                    hd.length - padlen,
-                                    padlen);
-          if(nghttp2_is_fatal(r)) {
-            return r;
-          }
-          framebuflen = session->aob.framebufoff + frame->hd.length
-            + NGHTTP2_FRAME_HEAD_LENGTH;
-
-          nghttp2_frame_pack_frame_hd
-            (session->aob.framebuf + session->aob.framebufoff, &hd);
-        } else {
-          /* PAD_HIGH and PAD_LOW will be added in
-             nghttp2_session_after_frame_sent(). */
-          framebuflen += frame->headers.padlen;
-        }
-        /* At this point, framebuflen > session->aob.framebufmax. But
-           before we access the missing part, we will allocate it in
-           nghttp2_session_after_frame_sent(). */
-      } else if(frame->hd.length <= NGHTTP2_MAX_FRAME_LENGTH &&
-                frame->headers.padlen > 0) {
-        r = nghttp2_frame_add_pad(&session->aob.framebuf,
-                                  &session->aob.framebufmax,
-                                  &session->aob.framebufoff,
-                                  &frame->hd.flags,
-                                  frame->hd.length - frame->headers.padlen,
-                                  frame->headers.padlen);
-        if(nghttp2_is_fatal(r)) {
-          return r;
-        }
-        framebuflen = session->aob.framebufoff + frame->hd.length
-          + NGHTTP2_FRAME_HEAD_LENGTH;
-
-        nghttp2_frame_pack_frame_hd
-          (session->aob.framebuf + session->aob.framebufoff, &frame->hd);
       }
 
       switch(frame->headers.cat) {
@@ -1281,11 +1288,17 @@ static ssize_t nghttp2_session_prep_frame(nghttp2_session *session,
       session->next_stream_id += 2;
       framebuflen = nghttp2_frame_pack_push_promise(&session->aob.framebuf,
                                                     &session->aob.framebufmax,
+                                                    &session->aob.framebufoff,
                                                     &frame->push_promise,
                                                     &session->hd_deflater);
       if(framebuflen < 0) {
         return framebuflen;
       }
+      framebuflen = session_headers_add_pad(session, frame);
+      if(framebuflen < 0) {
+        return framebuflen;
+      }
+
       stream = nghttp2_session_get_stream(session, frame->hd.stream_id);
       assert(stream);
       if(nghttp2_session_open_stream
@@ -1538,6 +1551,9 @@ static int nghttp2_session_after_frame_sent(nghttp2_session *session)
           if(cont_hd.length < frame->headers.padlen) {
             padlen = cont_hd.length;
           } else {
+            /* We use frame->headers.padlen for PUSH_PROMISE too. This
+               is possible because padlen is located in the same
+               position. */
             padlen = frame->headers.padlen;
           }
           DEBUGF(fprintf(stderr,
@@ -3454,10 +3470,16 @@ static int inbound_frame_handle_pad(nghttp2_inbound_frame *iframe,
     if((hd->flags & NGHTTP2_FLAG_PAD_LOW) == 0) {
       return -1;
     }
+    if(hd->length < 2) {
+      return -1;
+    }
     inbound_frame_reset_left(iframe, 2);
     return 1;
   }
   if(hd->flags & NGHTTP2_FLAG_PAD_LOW) {
+    if(hd->length < 1) {
+      return -1;
+    }
     inbound_frame_reset_left(iframe, 1);
     return 1;
   }
@@ -3657,6 +3679,21 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session,
         break;
       case NGHTTP2_PUSH_PROMISE:
         DEBUGF(fprintf(stderr, "PUSH_PROMISE\n"));
+        rv = inbound_frame_handle_pad(iframe, &iframe->frame.hd);
+        if(rv < 0) {
+          busy = 1;
+          iframe->state = NGHTTP2_IB_IGN_PAYLOAD;
+          rv = nghttp2_session_terminate_session(session,
+                                                 NGHTTP2_PROTOCOL_ERROR);
+          if(nghttp2_is_fatal(rv)) {
+            return rv;
+          }
+          break;
+        }
+        if(rv == 1) {
+          iframe->state = NGHTTP2_IB_READ_NBYTE;
+          break;
+        }
         if(iframe->payloadleft < 4) {
           busy = 1;
           iframe->state = NGHTTP2_IB_FRAME_SIZE_ERROR;
@@ -3762,6 +3799,29 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session,
         nghttp2_inbound_frame_reset(session);
         break;
       case NGHTTP2_PUSH_PROMISE:
+        if(iframe->padlen == 0 &&
+           iframe->frame.hd.flags & NGHTTP2_FLAG_PAD_LOW) {
+          rv = inbound_frame_compute_pad(iframe);
+          if(rv < 0) {
+            busy = 1;
+            rv = nghttp2_session_terminate_session(session,
+                                                   NGHTTP2_PROTOCOL_ERROR);
+            if(nghttp2_is_fatal(rv)) {
+              return rv;
+            }
+            iframe->state = NGHTTP2_IB_IGN_PAYLOAD;
+            break;
+          }
+          iframe->frame.push_promise.padlen = rv;
+          if(iframe->payloadleft < 4) {
+            busy = 1;
+            iframe->state = NGHTTP2_IB_FRAME_SIZE_ERROR;
+            break;
+          }
+          iframe->state = NGHTTP2_IB_READ_NBYTE;
+          inbound_frame_reset_left(iframe, 4);
+          break;
+        }
         rv = session_process_push_promise_frame(session);
         if(nghttp2_is_fatal(rv)) {
           return rv;

tests/nghttp2_frame_test.c

@@ -263,10 +263,12 @@ void test_nghttp2_frame_pack_push_promise()
   nghttp2_push_promise frame, oframe;
   uint8_t *buf = NULL;
   size_t buflen = 0;
+  size_t bufoff;
   ssize_t framelen;
   nghttp2_nv *nva;
   ssize_t nvlen;
   nva_out out;
+  ssize_t nv_offset;
 
   nva_out_init(&out);
   nghttp2_hd_deflate_init(&deflater);
@@ -276,16 +278,19 @@ void test_nghttp2_frame_pack_push_promise()
   nvlen = HEADERS_LENGTH;
   nghttp2_frame_push_promise_init(&frame, NGHTTP2_FLAG_END_PUSH_PROMISE,
                                   1000000007, (1U << 31) - 1, nva, nvlen);
-  framelen = nghttp2_frame_pack_push_promise(&buf, &buflen, &frame, &deflater);
+  framelen = nghttp2_frame_pack_push_promise(&buf, &buflen, &bufoff, &frame,
+                                             &deflater);
 
-  CU_ASSERT(0 == unpack_frame((nghttp2_frame*)&oframe, buf, framelen));
+  CU_ASSERT(0 == unpack_frame((nghttp2_frame*)&oframe,
-  check_frame_header(framelen - NGHTTP2_FRAME_HEAD_LENGTH,
+                              buf + bufoff, framelen - bufoff));
+  check_frame_header(framelen - bufoff - NGHTTP2_FRAME_HEAD_LENGTH,
                      NGHTTP2_PUSH_PROMISE,
                      NGHTTP2_FLAG_END_PUSH_PROMISE, 1000000007, &oframe.hd);
   CU_ASSERT((1U << 31) - 1 == oframe.promised_stream_id);
 
-  CU_ASSERT(framelen - 12 ==
+  nv_offset = bufoff + NGHTTP2_FRAME_HEAD_LENGTH + 4;
-            inflate_hd(&inflater, &out, buf + 12, framelen - 12));
+  CU_ASSERT(framelen - nv_offset ==
+            inflate_hd(&inflater, &out, buf + nv_offset, framelen - nv_offset));
 
   CU_ASSERT(7 == out.nvlen);
   CU_ASSERT(nvnameeq("method", &out.nva[0]));

tests/nghttp2_session_test.c

@@ -4044,7 +4044,7 @@ void test_nghttp2_session_pack_headers_with_padding(void)
   nghttp2_session *session, *sv_session;
   accumulator acc;
   my_user_data ud;
-  nghttp2_session_callbacks callbacks, sv_callbacks;
+  nghttp2_session_callbacks callbacks;
   nghttp2_nv nva[8190];
   size_t i;
 
@@ -4059,15 +4059,13 @@ void test_nghttp2_session_pack_headers_with_padding(void)
   callbacks.send_callback = accumulator_send_callback;
   callbacks.on_frame_send_callback = on_frame_send_callback;
   callbacks.select_padding_callback = select_padding_callback;
+  callbacks.on_frame_recv_callback = on_frame_recv_callback;
 
   acc.length = 0;
   ud.acc = &acc;
 
-  memset(&sv_callbacks, 0, sizeof(sv_callbacks));
-  sv_callbacks.on_frame_recv_callback = on_frame_recv_callback;
-
   nghttp2_session_client_new(&session, &callbacks, &ud);
-  nghttp2_session_server_new(&sv_session, &sv_callbacks, &ud);
+  nghttp2_session_server_new(&sv_session, &callbacks, &ud);
 
   ud.padding_boundary = 16385;
 
@@ -4083,6 +4081,20 @@ void test_nghttp2_session_pack_headers_with_padding(void)
   CU_ASSERT(1 == ud.frame_recv_cb_called);
   CU_ASSERT(NULL == nghttp2_session_get_next_ob_item(sv_session));
 
+  /* Check PUSH_PROMISE */
+  CU_ASSERT(0 ==
+            nghttp2_submit_push_promise(sv_session, NGHTTP2_FLAG_NONE, 1,
+                                        nva, ARRLEN(nva)));
+  acc.length = 0;
+  CU_ASSERT(0 == nghttp2_session_send(sv_session));
+
+  CU_ASSERT(acc.length > NGHTTP2_MAX_FRAME_LENGTH);
+  ud.frame_recv_cb_called = 0;
+  CU_ASSERT((ssize_t)acc.length ==
+            nghttp2_session_mem_recv(session, acc.buf, acc.length));
+  CU_ASSERT(1 == ud.frame_recv_cb_called);
+  CU_ASSERT(NULL == nghttp2_session_get_next_ob_item(session));
+
   nghttp2_session_del(sv_session);
   nghttp2_session_del(session);
 }