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

["Sudheer Vinukonda (JIRA)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/TS-3286?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Sudheer Vinukonda updated TS-3286:
----------------------------------
    Fix Version/s:     (was: 6.1.0)
                   6.2.0

> 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
>            Assignee: Sudheer Vinukonda
>             Fix For: 6.2.0
>
>
> 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)