Skip to content

Commit

Permalink
Fixed CORE-5294: Memory leak when use SHOW GRANTS on new empty databa…
Browse files Browse the repository at this point in the history
…se (3.0.1 & 4.0; SS & SC), also updated internal memleaks search tool
  • Loading branch information
AlexPeshkoff committed Jul 5, 2016
1 parent b247a5c commit ee105dd
Show file tree
Hide file tree
Showing 7 changed files with 110 additions and 18 deletions.
2 changes: 0 additions & 2 deletions src/auth/SecureRemotePassword/Message.h
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,6 @@ class Message
getMetadataBuilder(&statusWrapper, 0);
check(&statusWrapper);
builder = bld;
builder->addRef();
}
}
catch (...)
Expand Down Expand Up @@ -200,7 +199,6 @@ class Message
Firebird::IMessageMetadata* aMeta = builder->getMetadata(&statusWrapper);
check(&statusWrapper);
metadata = aMeta;
metadata->addRef();
builder->release();
builder = NULL;
}
Expand Down
50 changes: 46 additions & 4 deletions src/common/classes/ImplementHelper.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@
#include "../common/classes/RefCounted.h"
#include "../common/StatusArg.h"
#include "consts_pub.h"
#ifdef DEV_BUILD
#include <stdio.h>
#endif

namespace Firebird {

Expand Down Expand Up @@ -84,28 +87,62 @@ class DisposeIface : public VersionedIface<C>, public GlobalStorage
template <class C>
class RefCntIface : public VersionedIface<C>, public GlobalStorage
{
#ifdef DEV_BUILD

public:
RefCntIface() : refCounter(0) { }
RefCntIface(const char* M = NULL)
: refCounter(0), mark(M)
{
RefCntDPrt('^');
}

#ifdef DEV_BUILD
protected:
~RefCntIface()
{
RefCntDPrt('_');
fb_assert(refCounter.value() == 0);
}

public:
#endif

void addRef()
{
RefCntDPrt('+');
++refCounter;
}

//// TODO: can move release method to here, cause C has virtual destructor.

protected:

void RefCntDPrt(char f)
{
if (mark)
fprintf(stderr, "%s %p %c %lld\n", mark, this, f, refCounter.value());
}

AtomicCounter refCounter;
const char* mark;

#else

public:
RefCntIface() : refCounter(0) { }

void addRef()
{
++refCounter;
}

//// TODO: can move release method to here, cause C has virtual destructor.

protected:
AtomicCounter refCounter;

void RefCntDPrt(char)
{ }

#endif
};


Expand All @@ -117,7 +154,12 @@ class StdPlugin : public RefCntIface<C>
IReferenceCounted* owner;

public:
StdPlugin() : owner(NULL)
StdPlugin(const char* M = NULL)
#ifdef DEV_BUILD
: RefCntIface<C>(M), owner(NULL)
#else
: owner(NULL)
#endif
{ }

IReferenceCounted* getOwner()
Expand Down
36 changes: 36 additions & 0 deletions src/common/classes/init.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,14 @@
#include "alloc.h"
#include "../common/SimpleStatusVector.h"
#include "../common/dllinst.h"
#include "../common/os/os_utils.h"

#ifdef HAVE_DLADDR
#ifndef _GNU_SOURCE
#define _GNU_SOURCE
#endif
#include <dlfcn.h>
#endif // HAVE_DLADDR

// Setting this define helps (with AV at exit time) detect globals
// with destructors, declared not using InstanceControl.
Expand Down Expand Up @@ -100,6 +108,34 @@ namespace

try
{
#ifdef DEBUG_GDS_ALLOC
// In Debug mode - this will report all server-side memory leaks due to remote access
FILE* file = NULL;
{
Firebird::PathName name = fb_utils::getPrefix(
Firebird::IConfigManager::DIR_LOG, "memdebug.log");
#ifdef HAVE_DLADDR
Dl_info path;
if (dladdr((void *)(&allClean), &path))
{
name = path.dli_fname;
name += ".memdebug";
}
else
{
fprintf(stderr, "dladdr: %s\n", dlerror());
}
#endif
file = os_utils::fopen(name.c_str(), "w+t");
}

if (file)
{
getDefaultMemoryPool()->print_contents(file,
Firebird::MemoryPool::PRINT_USED_ONLY | Firebird::MemoryPool::PRINT_RECURSIVE);
fclose(file);
}
#endif
Firebird::MemoryPool::cleanup();
}
catch (...)
Expand Down
10 changes: 5 additions & 5 deletions src/jrd/DbCreators.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ bool openDb(const char* securityDb, RefPtr<IAttachment>& att, RefPtr<ITransactio

FbLocalStatus st;
DispatcherPtr prov;
att = prov->attachDatabase(&st, securityDb,
embeddedAttach.getBufferLength(), embeddedAttach.getBuffer());
att.assignRefNoIncr(prov->attachDatabase(&st, securityDb,
embeddedAttach.getBufferLength(), embeddedAttach.getBuffer()));
if (st->getState() & IStatus::STATE_ERRORS)
{
if (!fb_utils::containsErrorCode(st->getErrors(), isc_io_error))
Expand All @@ -91,7 +91,7 @@ bool openDb(const char* securityDb, RefPtr<IAttachment>& att, RefPtr<ITransactio
ClumpletWriter readOnly(ClumpletWriter::Tpb, MAX_DPB_SIZE, isc_tpb_version1);
readOnly.insertTag(isc_tpb_read);
readOnly.insertTag(isc_tpb_wait);
tra = att->startTransaction(&st, readOnly.getBufferLength(), readOnly.getBuffer());
tra.assignRefNoIncr(att->startTransaction(&st, readOnly.getBufferLength(), readOnly.getBuffer()));
check("IAttachment::startTransaction", &st);

return true;
Expand Down Expand Up @@ -237,7 +237,7 @@ bool checkCreateDatabaseGrant(const MetaName& userName, const MetaName& trustedR
" where p.rdb$privilege = 'M' and (p.rdb$field_name = 'D' or t.ur = 1)) "
"select r.rdb$system_privileges "
" from role_tree t join rdb$roles r on t.nm = r.rdb$role_name ";
RefPtr<IResultSet> rs(att->openCursor(&st, tra, 0, sql,
RefPtr<IResultSet> rs(REF_NO_INCR, att->openCursor(&st, tra, 0, sql,
SQL_DIALECT_V6, par2.getMetadata(), par2.getBuffer(), res2.getMetadata(), NULL, 0));
check("IAttachment::execute", &st);

Expand Down Expand Up @@ -307,7 +307,7 @@ RecordBuffer* DbCreatorsList::getList(thread_db* tdbb, jrd_rel* relation)
Field<Varying> u(gr, MAX_SQL_IDENTIFIER_LEN);

FbLocalStatus st;
RefPtr<IResultSet> curs(att->openCursor(&st, tra, 0,
RefPtr<IResultSet> curs(REF_NO_INCR, att->openCursor(&st, tra, 0,
"select RDB$USER_TYPE, RDB$USER from RDB$DB_CREATORS",
SQL_DIALECT_V6, NULL, NULL, gr.getMetadata(), NULL, 0));

Expand Down
16 changes: 11 additions & 5 deletions src/remote/server/server.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -981,7 +981,7 @@ static void ping_connection(rem_port*, PACKET*);
static bool process_packet(rem_port* port, PACKET* sendL, PACKET* receive, rem_port** result);
static void release_blob(Rbl*);
static void release_event(Rvnt*);
static void release_request(Rrq*);
static void release_request(Rrq*, bool rlsIface = false);
static void release_statement(Rsr**);
static void release_sql_request(Rsr*);
static void release_transaction(Rtr*);
Expand Down Expand Up @@ -2711,7 +2711,7 @@ void rem_port::disconnect(PACKET* sendL, PACKET* receiveL)
rdb->rdb_iface->cancelOperation(&status_vector, fb_cancel_disable);

while (rdb->rdb_requests)
release_request(rdb->rdb_requests);
release_request(rdb->rdb_requests, true);

while (rdb->rdb_sql_requests)
release_sql_request(rdb->rdb_sql_requests);
Expand Down Expand Up @@ -2828,7 +2828,7 @@ void rem_port::drop_database(P_RLSE* /*release*/, PACKET* sendL)
release_event(rdb->rdb_events);

while (rdb->rdb_requests)
release_request(rdb->rdb_requests);
release_request(rdb->rdb_requests, true);

while (rdb->rdb_sql_requests)
release_sql_request(rdb->rdb_sql_requests);
Expand Down Expand Up @@ -2910,7 +2910,7 @@ ISC_STATUS rem_port::end_database(P_RLSE* /*release*/, PACKET* sendL)
rdb->rdb_iface = NULL;

while (rdb->rdb_requests)
release_request(rdb->rdb_requests);
release_request(rdb->rdb_requests, true);

while (rdb->rdb_sql_requests)
release_sql_request(rdb->rdb_sql_requests);
Expand Down Expand Up @@ -4950,7 +4950,7 @@ static void release_event( Rvnt* event)
}


static void release_request( Rrq* request)
static void release_request( Rrq* request, bool rlsIface)
{
/**************************************
*
Expand All @@ -4962,6 +4962,12 @@ static void release_request( Rrq* request)
* Release a request block.
*
**************************************/
if (rlsIface && request->rrq_iface)
{
request->rrq_iface->release();
request->rrq_iface = NULL;
}

Rdb* rdb = request->rrq_rdb;
rdb->rdb_port->releaseObject(request->rrq_id);
REMOTE_release_request(request);
Expand Down
9 changes: 7 additions & 2 deletions src/yvalve/YObjects.h
Original file line number Diff line number Diff line change
Expand Up @@ -121,18 +121,23 @@ class HandleArray
};

template <typename Impl, typename Intf>
class YHelper : public Firebird::StdPlugin<Intf>, public YObject
class YHelper : public Firebird::RefCntIface<Intf>, public YObject
{
public:
typedef typename Intf::Declaration NextInterface;
typedef YAttachment YRef;

static const unsigned DF_RELEASE = 0x1;

explicit YHelper(NextInterface* aNext);
explicit YHelper(NextInterface* aNext
#ifdef DEV_BUILD
, const char* M = NULL

This comment has been minimized.

Copy link
@asfernandes

asfernandes Jul 11, 2016

Member

Why do use parameter names in all upper case, not matching standard name convention?

#endif
);

int release()
{
Firebird::RefCntIface<Intf>::RefCntDPrt('-');
int rc = --(this->refCounter);
if (rc == 0)
{
Expand Down
5 changes: 5 additions & 0 deletions src/yvalve/why.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3846,7 +3846,12 @@ ITransaction* MasterImplementation::registerTransaction(IAttachment* attachment,
}

template <typename Impl, typename Intf>
#ifdef DEV_BUILD
YHelper<Impl, Intf>::YHelper(NextInterface* aNext, const char* Mark)
: RefCntIface<Intf>(Mark)
#else
YHelper<Impl, Intf>::YHelper(NextInterface* aNext)
#endif
{
next.assignRefNoIncr(aNext);
}
Expand Down

0 comments on commit ee105dd

Please sign in to comment.