[ 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.2.0) 7.0.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: 7.0.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)