Uploaded image for project: 'Traffic Server'
  1. Traffic Server
  2. TS-3845

HPACK error when trying to delete entries from an empty table

    XMLWordPrintableJSON

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.0.0
    • Component/s: HTTP/2
    • Labels:
      None

      Description

      This coredump that happens very seldom. It happens when Http2DynamicTable::add_header_field() is called and it tires to delete entries from an empty table to make room.

      Here is a patch to stop the core from happening, but it would be better to find the root cause. I am running another patch in production that walks the table on every add and calculates the used space and compares it to _current_size to see if I can diagnose it more.

      diff --git a/proxy/http2/HPACK.cc b/proxy/http2/HPACK.cc
      index d65a6b1..76e3877 100644
      --- a/proxy/http2/HPACK.cc
      +++ b/proxy/http2/HPACK.cc
      @@ -230,7 +230,7 @@ Http2DynamicTable::set_dynamic_table_size(uint32_t new_size)
         return true;
       }
      
      -void
      +bool
       Http2DynamicTable::add_header_field(const MIMEField *field)
       {
         int name_len, value_len;
      @@ -247,7 +247,7 @@ Http2DynamicTable::add_header_field(const MIMEField *field)
           _mhdr->fields_clear();
         } else {
           _current_size += header_size;
      -    while (_current_size > _settings_dynamic_table_size) {
      +    while (_current_size > _settings_dynamic_table_size && _headers.length() > 0) {
             int last_name_len, last_value_len;
             MIMEField *last_field = _headers.last();
      
      @@ -259,11 +259,18 @@ Http2DynamicTable::add_header_field(const MIMEField *field)
             _mhdr->field_delete(last_field, false);
           }
      
      +    if (_headers.length() == 0 && _current_size - header_size != 0) {
      +      Error("No headers in dynamic table, but the size is: %u", _current_size);
      +      return false; // this will close the connection
      +    }
      +
           MIMEField *new_field = _mhdr->field_create(name, name_len);
           new_field->value_set(_mhdr->m_heap, _mhdr->m_mime, value, value_len);
           // XXX Because entire Vec instance is copied, Its too expensive!
           _headers.insert(0, new_field);
         }
      +
      +  return true;
       }
      
       // The first byte of an HPACK field unambiguously tells us what
      @@ -658,7 +665,9 @@ decode_literal_header_field(MIMEFieldWrapper &header, const uint8_t *buf_start,
      
         // Incremental Indexing adds header to header table as new entry
         if (isIncremental) {
      -    dynamic_table.add_header_field(header.field_get());
      +    if (dynamic_table.add_header_field(header.field_get()) == false) {
      +      return HPACK_ERROR_COMPRESSION_ERROR;
      +    }
         }
      
         // Print decoded header field
      diff --git a/proxy/http2/HPACK.h b/proxy/http2/HPACK.h
      index 4e63a37..e03ef1a 100644
      --- a/proxy/http2/HPACK.h
      +++ b/proxy/http2/HPACK.h
      @@ -111,7 +111,7 @@ public:
           delete _mhdr;
         }
      
      -  void add_header_field(const MIMEField *field);
      +  bool add_header_field(const MIMEField *field);
         int get_header_from_indexing_tables(uint32_t index, MIMEFieldWrapper &header_field) const;
         bool set_dynamic_table_size(uint32_t new_size);
      

        Attachments

          Activity

            People

            • Assignee:
              zwoop Leif Hedstrom
              Reporter:
              bcall Bryan Call
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: