Skip to content

[asan][aclorch] fix a memory leak in the SaiAttrWrapper::swap()#2382

Merged
prsunny merged 4 commits intosonic-net:masterfrom
Yakiv-Huryk:saiattrmovefix
Aug 9, 2022
Merged

[asan][aclorch] fix a memory leak in the SaiAttrWrapper::swap()#2382
prsunny merged 4 commits intosonic-net:masterfrom
Yakiv-Huryk:saiattrmovefix

Conversation

@Yakiv-Huryk
Copy link
Copy Markdown
Contributor

@Yakiv-Huryk Yakiv-Huryk commented Jul 19, 2022

fix a leak caused by overriding this->m_attr (which contained a dynamically allocated list) in the SaiAttrWrapper::swap()

ASAN report:

orchagent

Direct leak of 160 byte(s) in 9 object(s) allocated from:
    #0 0x7fd899555ef0 in operator new[](unsigned long) (/usr/lib/x86_64-linux-gnu/libasan.so.5+0xeaef0)
    #1 0x7fd898ea2b77 in unsigned long* sai_alloc_n_of_ptr_type<unsigned long, unsigned int>(unsigned int, unsigned long*) (/usr/lib/x86_64-linux-gnu/libsaimeta.so.0+0x1f0b77)
    #2 0x7fd898e96d68  (/usr/lib/x86_64-linux-gnu/libsaimeta.so.0+0x1e4d68)
    #3 0x7fd898e7fcf8 in sai_deserialize_oid_list(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, _sai_object_list_t&, bool) (/usr/lib/x86_64-linux-gnu/libsaimeta.so.0+0x1cdcf8)
    #4 0x7fd898e840c4 in sai_deserialize_acl_field(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, _sai_attr_metadata_t const&, _sai_acl_field_data_t&, bool) (/usr/lib/x86_64-linux-gnu/libsaimeta.so.0+0x1d20c4)
    #5 0x7fd898e87124 in sai_deserialize_attr_value(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, _sai_attr_metadata_t const&, _sai_attribute_t&, bool) (/usr/lib/x86_64-linux-gnu/libsaimeta.so.0+0x1d5124)
    #6 0x56209e54f2b0 in SaiAttrWrapper::init(_sai_object_type_t, _sai_attr_metadata_t const&, _sai_attribute_t const&) orchagent/saiattr.cpp:90
    #7 0x56209e54f791 in SaiAttrWrapper::SaiAttrWrapper(_sai_object_type_t, _sai_attribute_t const&) orchagent/saiattr.cpp:14
    #8 0x56209e4391b2 in AclRule::setMatch(_sai_acl_entry_attr_t, _sai_acl_field_data_t) orchagent/aclorch.cpp:1402
    #9 0x56209e435c6a in AclRule::validateAddMatch(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) orchagent/aclorch.cpp:950
    #10 0x56209e457a41 in AclOrch::updateAclRule(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, void*, bool) orchagent/aclorch.cpp:3948
    #11 0x56209e898961 in MuxAclHandler::MuxAclHandler(unsigned long, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) orchagent/muxorch.cpp:785
    #12 0x56209e899f8e in std::shared_ptr<MuxAclHandler> std::make_shared<MuxAclHandler, unsigned long&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&>(unsigned long&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) /usr/include/c++/8/ext/new_allocator.h:136
    #13 0x56209e899f8e in MuxCable::aclHandler(unsigned long, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool) orchagent/muxorch.cpp:493
    #14 0x56209e8b2ad8 in MuxCable::stateStandby() orchagent/muxorch.cpp:425
    #15 0x56209e8a48a7 in MuxCable::setState(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) orchagent/muxorch.cpp:462
    #16 0x56209e8a58d6 in MuxCableOrch::addOperation(Request const&) orchagent/muxorch.cpp:1470
    #17 0x56209dfef07f in Orch2::doTask(Consumer&) orchagent/orch.cpp:1067
    #18 0x56209dff66e6 in Consumer::execute() orchagent/orch.cpp:235
    #19 0x56209dfc5e15 in OrchDaemon::start() orchagent/orchdaemon.cpp:723
    #20 0x56209de66527 in main orchagent/main.cpp:734
    #21 0x7fd897e7609a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)

What I did
Fixed the logic in the SaiAttrWrapper::swap()

Why I did it
To fix a memory leak

How I verified it
Run a DVS test with ASAN that found the bug

Details if related

* fix a leak caused by overriding this->m_attr (which contained
a dynamically allocated list) in the SaiAttrWrapper::swap()

Signed-off-by: Yakiv Huryk <yhuryk@nvidia.com>
@Yakiv-Huryk Yakiv-Huryk requested a review from prsunny as a code owner July 19, 2022 15:45
@prsunny
Copy link
Copy Markdown
Collaborator

prsunny commented Aug 5, 2022

@Yakiv-Huryk , is there any existing test that covers this part of the code? @theasianpianist , Is this due to optimization?

@theasianpianist
Copy link
Copy Markdown
Contributor

@Yakiv-Huryk , is there any existing test that covers this part of the code? @theasianpianist , Is this due to optimization?

Probably an optimization issue, looks like the method is being hit but the object code from the compiler does not line up exactly with the source code.

@liat-grozovik
Copy link
Copy Markdown
Collaborator

/EasyCLA

@liat-grozovik
Copy link
Copy Markdown
Collaborator

@Yakiv-Huryk , is there any existing test that covers this part of the code? @theasianpianist , Is this due to optimization?

Probably an optimization issue, looks like the method is being hit but the object code from the compiler does not line up exactly with the source code.

in this case can we merge it regardless?

@liat-grozovik
Copy link
Copy Markdown
Collaborator

/easycla

@prsunny prsunny merged commit d36c17d into sonic-net:master Aug 9, 2022
@prsunny
Copy link
Copy Markdown
Collaborator

prsunny commented Aug 9, 2022

Merging as the diff coverage possibly is impacted by optimization.

Janetxxx pushed a commit to Janetxxx/sonic-swss that referenced this pull request Nov 10, 2025
…c-net#2382)

* fix a leak caused by overriding this->m_attr (which contained a dynamically allocated list) in the SaiAttrWrapper::swap()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants