You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficserver.apache.org by "Leif Hedstrom (JIRA)" <ji...@apache.org> on 2016/04/08 19:40:25 UTC
[jira] [Commented] (TS-3286) Improve MIOBuffer allocation mechanism
with checks to prevent corrupting the freelist
[ https://issues.apache.org/jira/browse/TS-3286?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15232546#comment-15232546 ]
Leif Hedstrom commented on TS-3286:
-----------------------------------
[~sudheerv] Are you going to land this for 6.2.0? If not, please move it out.
> 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)