IDTOKENS code aborts unnecessarily
Description
is duplicated by
Activity
John (TJ) Knoeller May 3, 2021 at 7:35 PM
I judged that 64k was too big for a stack buffer.
Todd L Miller May 3, 2021 at 3:11 PMEdited
Code Review
Not sure why the std::vector<char> in DataReuseDirectory::CacheFile() became a unique_ptr to an unchecked heap allocation instead of a character array on the stack; and the same for DataReuseDirectory::RetrieveFile(). Did I miss something? (It seems like it’d be marginally safer as char memory_buffer[1024*64];; is 64k too much for a stack allocation on some machine(s)?)
All of the other changes look good.
John (TJ) Knoeller April 29, 2021 at 5:08 PM
misuse of vector::reserve appears in these placessrc/condor_utils/data_reuse.cpp lines (577, 594, 633, 806, 843)src/condor_utils/token_utils.cpp lines (101)src/condor_io/condor_auth_ssl.cpp lines (595, 954) src/condor_tools/token_request.cpp lines (77)
John (TJ) Knoeller April 29, 2021 at 5:03 PMEdited
The full list of clang matches
/home/gthain/condor/src/condor_utils/compat_classad_util.cpp:923:3: note: "root" binds here
matches.reserve(matched);
^~~~~~~~~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_utils/condor_query.cpp:378:34: note: "root" binds here
std::vector<std::string> attrs; attrs.reserve(7);
^~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_utils/data_reuse.cpp:577:9: note: "root" binds here
dest_tmp_fname.reserve(dest_fname_template.size() + 1);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_utils/data_reuse.cpp:594:2: note: "root" binds here
memory_buffer.reserve(64*1024);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_utils/data_reuse.cpp:633:2: note: "root" binds here
computed_checksum.reserve(2*md_len + 1);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_utils/data_reuse.cpp:806:2: note: "root" binds here
memory_buffer.reserve(64*1024);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_utils/data_reuse.cpp:843:2: note: "root" binds here
computed_checksum.reserve(2*md_len + 1);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_utils/token_utils.cpp:101:2: note: "root" binds here
hostname.reserve(MAXHOSTNAMELEN);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_io/condor_auth_ssl.cpp:595:4: note: "root" binds here
network_bytes.reserve(sizeof(network_size) + scitoken.size());
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_io/condor_auth_ssl.cpp:954:5: note: "root" binds here
token_contents.reserve(m_auth_state->m_token_length + sizeof(uint32_t));
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_daemon_client/dc_starter.cpp:469:46: note: "root" binds here
std::vector<classad::ExprTree *> filelist; filelist.reserve(filenames.size());
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_daemon_client/dc_starter.cpp:470:48: note: "root" binds here
std::vector<classad::ExprTree *> offsetlist; offsetlist.reserve(filenames.size());
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_dagman/job.h:892:3: note: "root" binds here
_ary.reserve(num);
^~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_dagman/job.h:892:3: note: "root" binds here
_ary.reserve(num);
^~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_dagman/job.h:892:3: note: "root" binds here
_ary.reserve(num);
^~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_dagman/job.h:892:3: note: "root" binds here
_ary.reserve(num);
^~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_dagman/job.h:892:3: note: "root" binds here
_ary.reserve(num);
^~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_dagman/job.h:892:3: note: "root" binds here
_ary.reserve(num);
^~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_dagman/job.h:892:3: note: "root" binds here
_ary.reserve(num);
^~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_dagman/job.h:892:3: note: "root" binds here
_ary.reserve(num);
^~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_dagman/job.cpp:922:3: note: "root" binds here
edge->_ary.reserve(num_children);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_dagman/job.h:892:3: note: "root" binds here
_ary.reserve(num);
^~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_dagman/job.h:892:3: note: "root" binds here
_ary.reserve(num);
^~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_dagman/job.h:892:3: note: "root" binds here
_ary.reserve(num);
^~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_dagman/job.h:892:3: note: "root" binds here
_ary.reserve(num);
^~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_dagman/job.h:892:3: note: "root" binds here
_ary.reserve(num);
^~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_negotiator.V6/matchmaker.cpp:4380:3: note: "root" binds here
par_candidates.reserve(startdAds.Length());
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_negotiator.V6/matchmaker.cpp:4380:3: note: "root" binds here
par_candidates.reserve(startdAds.Length());
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_negotiator.V6/matchmaker.cpp:4380:3: note: "root" binds here
par_candidates.reserve(startdAds.Length());
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_negotiator.V6/matchmaker.cpp:4380:3: note: "root" binds here
par_candidates.reserve(startdAds.Length());
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_q.V6/jobmatch.cpp:1268:4: note: "root" binds here
ads.reserve(startdAds.size());
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/gthain/condor/src/condor_tools/token_request.cpp:77:2: note: "root" binds here
hostname.reserve(MAXHOSTNAMELEN);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/gthain/condor/src/gpu/opencl_device_enumeration.cpp:151:3: note: "root" binds here
cl_platforms.reserve(cPlatforms);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/gthain/condor/src/gpu/opencl_device_enumeration.cpp:184:4: note: "root" binds here
cl_gpu_ids.reserve(ixFirst + cDevs);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/gthain/condor/src/python-bindings/collector_plugin.cpp:111:5: note: "root" binds here
m_shutdown_funcs.reserve(modules.number());
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/gthain/condor/src/python-bindings/collector_plugin.cpp:112:5: note: "root" binds here
m_update_funcs.reserve(modules.number());
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/gthain/condor/src/python-bindings/collector_plugin.cpp:113:5: note: "root" binds here
m_invalidate_funcs.reserve(modules.number());
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/gthain/condor/src/python-bindings/collector.cpp:375:13: note: "root" binds here
attrs_str.reserve(len_attrs);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/gthain/condor/src/python-bindings/credd.cpp:697:3: note: "root" binds here
requests.reserve(num_req);
^~~~~~~~~~~~~~~~~~~~~~~~~
note that a simple grep for .reserve resulted in only 117 matches, which is a bit more to look through, but not too hard. It took about 20 minutes to look through in DevStudio and identify the problem uses, so the Clang search, while impressive, was not really more efficient for those not already using clang.
FYI: If you see a log entry that looks like this:
/usr/include/c++/8/bits/stl_vector.h:932: std::vector<_Tp, _Alloc>::reference std::vector<_Tp, _Alloc>::operator[](std::vector<_Tp, _Alloc>::size_type) [with _Tp = char; _Alloc = std::allocator<char>; std::vector<_Tp, _Alloc>::reference = char&; std::vector<_Tp, _Alloc>::size_type = long unsigned int]: Assertion '__builtin_expect(__n < this->size(), true)' failed. AbortedYou are probably affected by this bug.
Manuel Giffles wrote (about
condor_utils/token_utils.cpp):We need to find all the other instances of this antipattern (construct an empty vector on the stack, call
reserve(), and then write to the underlying storage) and either convert them to callresize()or just use astd::arraywith the correct initial size.See https://htcondor-wiki.cs.wisc.edu/index.cgi/wiki?p=LanguageAwareGreppingWithClangQuery for a way to find all such calls.