Skip to content

Make HostDBInfo class safer to use. #6858

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ywkaras merged 1 commit into from
Jun 26, 2020
Merged

Make HostDBInfo class safer to use. #6858

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 37 additions & 9 deletions iocore/hostdb/I_HostDBProcessor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,16 +146,17 @@ struct HostDBInfo : public RefCountObj {
ink_release_assert(iobuffer_index >= 0);
void *ptr = ioBufAllocator[iobuffer_index].alloc_void();
memset(ptr, 0, size);
HostDBInfo *ret = new (ptr) HostDBInfo();
ret->iobuffer_index = iobuffer_index;
HostDBInfo *ret = new (ptr) HostDBInfo();
ret->_iobuffer_index = iobuffer_index;
return ret;
}

void
free() override
{
Debug("hostdb", "freeing %d bytes at [%p]", (1 << (7 + iobuffer_index)), this);
ioBufAllocator[iobuffer_index].free_void((void *)(this));
ink_release_assert(from_alloc());
Copy link
Member

@zizhong zizhong Jun 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like, the diff introduces new ink_release_assert to make sure the object is allocated from HostDBInfo::alloc().
Could we simply the object uncopyable to achieve the same?

Copy link
Contributor Author

@ywkaras ywkaras Jun 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current code requires that instances of HostDBInfo be copyable. Not all instances of HostDBInfo are created by the alloc() function. It will be easier to debug accidental calls to free() with this assert, because without the assert, they will corrupt the freelist, and it's hard to find the cause of memory corruption.

Copy link
Member

@zizhong zizhong Jun 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current code requires that instances of HostDBInfo be copyable.

Could you elaborate on that? I'll be surprised to see if any HostDBInfo is copied around. ^_^

Copy link
Contributor Author

@ywkaras ywkaras Jun 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it the case I am aware of where a copy occurs:

trafficserver/proxy/http/HttpSM.cc

Line 2254 in af9c4f3

t_state.host_db_info = *ret;

Debug("hostdb", "freeing %d bytes at [%p]", (1 << (7 + _iobuffer_index)), this);
ioBufAllocator[_iobuffer_index].free_void((void *)(this));
}

// return a version number-- so we can manage compatibility with the marshal/unmarshal
Expand All @@ -172,12 +173,12 @@ struct HostDBInfo : public RefCountObj {
return nullptr;
}
HostDBInfo *ret = HostDBInfo::alloc(size - sizeof(HostDBInfo));
int buf_index = ret->iobuffer_index;
int buf_index = ret->_iobuffer_index;
memcpy((void *)ret, buf, size);
// Reset the refcount back to 0, this is a bit ugly-- but I'm not sure we want to expose a method
// to mess with the refcount, since this is a fairly unique use case
ret = new (ret) HostDBInfo();
ret->iobuffer_index = buf_index;
ret = new (ret) HostDBInfo();
ret->_iobuffer_index = buf_index;
return ret;
}

Expand Down Expand Up @@ -315,8 +316,6 @@ struct HostDBInfo : public RefCountObj {
}
}

int iobuffer_index;

uint64_t key;

// Application specific data. NOTE: We need an integral number of
Expand All @@ -341,6 +340,35 @@ struct HostDBInfo : public RefCountObj {

unsigned int round_robin : 1; // This is the root of a round robin block
unsigned int round_robin_elt : 1; // This is an address in a round robin block

HostDBInfo() : _iobuffer_index{-1} {}

HostDBInfo(HostDBInfo const &src) : RefCountObj()
{
memcpy(static_cast<void *>(this), static_cast<const void *>(&src), sizeof(*this));
_iobuffer_index = -1;
}

HostDBInfo &
operator=(HostDBInfo const &src)
{
if (this != &src) {
int iob_idx = _iobuffer_index;
memcpy(static_cast<void *>(this), static_cast<const void *>(&src), sizeof(*this));
_iobuffer_index = iob_idx;
}
return *this;
}

bool
from_alloc() const
{
return _iobuffer_index >= 0;
}

private:
// The value of this will be -1 for objects that are not created by the alloc() static member function.
int _iobuffer_index;
};

struct HostDBRoundRobin {
Expand Down