[jira] [Created] (TS-3286) Improve MIOBuffer allocation mechanism with checks to prevent corrupting the freelist

[Sudheer Vinukonda (JIRA)]

Sudheer Vinukonda created TS-3286:
-------------------------------------

             Summary: Improve MIOBuffer allocation mechanism with checks to 
prevent corrupting the freelist
                 Key: TS-3286
                 URL: https://issues.apache.org/jira/browse/TS-3286
             Project: Traffic Server
          Issue Type: Improvement
          Components: Core
            Reporter: Sudheer Vinukonda


This jira is a follow up on TS-3285. While investigating TS-3285, I realized 
that there are not sufficient guards in-place in the MIOBuffer 
allocation/access mechanism to prevent corrupting the free list or accessing 
the objects off of the freelist. 

These issues are particularly problematic, when creating the MIOBuffer using 
new_empty_MIOBuffer(). This simply pulls out the MIOBuffer from the freelist, 
without resetting/initializing the data members. So, if the MIOBuffer on the 
freelist is bad, then the newly allocated buffer will run into trouble (e.g 
TS-3285). Note that, if the MIOBuffer is allocated using new_MIOBuffer(), it 
calls clear() which resets the members correctly. 

The problem also is partly due to the fact that MIOBuffer uses ProxyAllocator 
and when the object is allocated from the local thread freelist, it is not 
reset with a prototype object memcpy (like the ClassAllocator would do).

Some checks/asserts I am thinking of adding are as below:

{code}
diff --git a/iocore/eventsystem/IOBuffer.cc b/iocore/eventsystem/IOBuffer.cc
index 879e8ba..d54080f 100644
--- a/iocore/eventsystem/IOBuffer.cc
+++ b/iocore/eventsystem/IOBuffer.cc
@@ -82,6 +82,7 @@ MIOBuffer::remove_append(IOBufferReader * r)
 int64_t
 MIOBuffer::write(const void *abuf, int64_t alen)
 {
+  ink_release_assert(size_index < BUFFER_SIZE_NOT_ALLOCATED);
   const char *buf = (const char*)abuf;
   int64_t len = alen;
   while (len) {
@@ -135,6 +136,7 @@ 
MIOBuffer::write_and_transfer_left_over_space(IOBufferReader * r, int64_t alen,
 int64_t
 MIOBuffer::write(IOBufferReader * r, int64_t alen, int64_t offset)
 {
+  ink_release_assert(size_index < BUFFER_SIZE_NOT_ALLOCATED);
   int64_t len = alen;
   IOBufferBlock *b = r->block;
   offset += r->start_offset;
diff --git a/iocore/eventsystem/I_IOBuffer.h b/iocore/eventsystem/I_IOBuffer.h
index 0e5fe0c..d9aba81 100644
--- a/iocore/eventsystem/I_IOBuffer.h
+++ b/iocore/eventsystem/I_IOBuffer.h
@@ -1126,8 +1126,6 @@ public:
     _writer->realloc_xmalloc(buf_size);
   }
 
-  int64_t size_index;
-
   /**
     Determines when to stop writing or reading. The watermark is the
     level to which the producer (filler) is required to fill the buffer
@@ -1138,6 +1136,8 @@ public:
   */
   int64_t water_mark;
 
+  int64_t size_index;
+
   Ptr<IOBufferBlock> _writer;
   IOBufferReader readers[MAX_MIOBUFFER_READERS];
 
diff --git a/iocore/eventsystem/P_IOBuffer.h b/iocore/eventsystem/P_IOBuffer.h
index 365486f..74ea432 100644
--- a/iocore/eventsystem/P_IOBuffer.h
+++ b/iocore/eventsystem/P_IOBuffer.h
@@ -775,8 +775,7 @@ TS_INLINE MIOBuffer * new_MIOBuffer_internal(
 TS_INLINE void
 free_MIOBuffer(MIOBuffer * mio)
 {
-  mio->_writer = NULL;
-  mio->dealloc_all_readers();
+  clear();
   THREAD_FREE(mio, ioAllocator, this_thread());
 }
 
@@ -893,6 +892,7 @@ MIOBuffer::append_block_internal(IOBufferBlock * b)
   // It would be nice to remove an empty buffer at the beginning,
   // but this breaks HTTP.
   // if (!_writer || !_writer->read_avail())
+  ink_release_assert(size_index < BUFFER_SIZE_NOT_ALLOCATED);
   if (!_writer) {
     _writer = b;
     init_readers();
{code}



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)