some more fixes

[neheb]

I'm not sure about the shared_ptr change. Cyclic references are avoided by passing by value. I doubt there's much overhead here.

[neheb]

more false positives. unique_ptr is not a vector.

ping @kevinbackhouse

[neheb]

some more make_uniques. Let's see if it complains now.

[kevinbackhouse]

@neheb: The "Unsafe Vector Access" CodeQL query is something that I wrote myself and it's definitely not perfect. The fuzzer found quite a few crashes that could have been prevented by using vector::at() because it does a bounds check and throws an exception. So my goal was just to find all the calls to vector::operator[] and replace them with vector::at().

Are some of these changes just attempts to silence the CodeQL alerts? For example, is replacing std::vector<char> with std::make_unique<char[]> a good change, or is it just silencing a warning? If it's just to silence a warning, then I would say it's unnecessary. I'm also not sure about replacing SharedPtr with UniquePtr. Isn't it better to use UniquePtr whenever possible? It makes the ownership easier to understand.

[neheb]

The last commit changes to unique_ptr as only a raw pointer is used. No vector functionality like push_back. Probably uses less memory.

As for the first, it looks like get() is just used to avoid having a second smart pointer. I don’t know if that’s a good idea.

[kevinbackhouse]

@neheb: I think #3137 will help. It removes the field so there's no need for a shared pointer.

[neheb]

I'll rebase once that's merged.

[kevinbackhouse]

The decodeHexTable change looks unnecessary to me. Other than that, everything else is good.

[neheb]

@kevinbackhouse I was right to question this get() usage. clang-analyzer complains with:

../src/crwimage_int.cpp:577:3: warning: Use of memory after it is freed [clang-analyzer-cplusplus.NewDelete]
  577 |   return cc;
      |   ^
../src/crwimage_int.cpp:992:7: note: Assuming the condition is true
  992 |   if (!buf.empty()) {
      |       ^~~~~~~~~~~~
../src/crwimage_int.cpp:992:3: note: Taking true branch
  992 |   if (!buf.empty()) {
      |   ^
../src/crwimage_int.cpp:993:5: note: Calling 'CiffHeader::add'
  993 |     pHead.add(pCrwMapping.crwTagId_, pCrwMapping.crwDir_, std::move(buf));
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../src/crwimage_int.cpp:516:3: note: Taking false branch
  516 |   if (!pRootDir_) {
      |   ^
../src/crwimage_int.cpp:519:20: note: Calling 'CiffComponent::add'
  519 |   if (auto child = pRootDir_->add(crwDirs, crwTagId)) {
      |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../src/crwimage_int.cpp:525:10: note: Calling 'CiffDirectory::doAdd'
  525 |   return doAdd(crwDirs, crwTagId);
      |          ^~~~~~~~~~~~~~~~~~~~~~~~
../src/crwimage_int.cpp:546:7: note: Assuming the condition is false
  546 |   if (!crwDirs.empty()) {
      |       ^~~~~~~~~~~~~~~~
../src/crwimage_int.cpp:546:3: note: Taking false branch
  546 |   if (!crwDirs.empty()) {
      |   ^
../src/crwimage_int.cpp:571:8: note: 'cc' is null
  571 |   if (!cc) {
      |        ^~
../src/crwimage_int.cpp:571:3: note: Taking true branch
  571 |   if (!cc) {
      |   ^
../src/crwimage_int.cpp:573:14: note: Calling 'make_unique<Exiv2::Internal::CiffEntry, unsigned short &, unsigned short>'
  573 |     auto m = std::make_unique<CiffEntry>(crwTagId, tag());
      |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/unique_ptr.h:962:30: note: Memory is allocated
  962 |     { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); }
      |                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../src/crwimage_int.cpp:573:14: note: Returned allocated memory
  573 |     auto m = std::make_unique<CiffEntry>(crwTagId, tag());
      |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../src/crwimage_int.cpp:575:5: note: Calling 'CiffComponent::add'
  575 |     add(std::move(m));
      |     ^~~~~~~~~~~~~~~~~
../src/crwimage_int.cpp:144:28: note: Calling '~unique_ptr'
  144 |   doAdd(std::move(component));
      |                            ^
/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/unique_ptr.h:360:2: note: Taking true branch
  360 |         if (__ptr != nullptr)
      |         ^
/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/unique_ptr.h:361:4: note: Calling 'default_delete::operator()'
  361 |           get_deleter()(std::move(__ptr));
      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/unique_ptr.h:85:2: note: Memory is released
   85 |         delete __ptr;
      |         ^~~~~~~~~~~~
/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/unique_ptr.h:361:4: note: Returning; memory was released via 2nd parameter
  361 |           get_deleter()(std::move(__ptr));
      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../src/crwimage_int.cpp:144:28: note: Returning from '~unique_ptr'
  144 |   doAdd(std::move(component));
      |                            ^
../src/crwimage_int.cpp:575:5: note: Returning; memory was released
  575 |     add(std::move(m));
      |     ^~~~~~~~~~~~~~~~~
../src/crwimage_int.cpp:577:3: note: Use of memory after it is freed
  577 |   return cc;

add() eventually calls release() on the pointer which also ends up freeing it. so m.get() returns garbage. Supposedly.

[kevinbackhouse]

@kevinbackhouse I was right to question this get() usage. clang-analyzer complains with:

@neheb: which log file is this in? Or can you give me instructions to reproduce it locally?

[neheb]

@kevinbackhouse I was right to question this get() usage. clang-analyzer complains with:

@neheb: which log file is this in? Or can you give me instructions to reproduce it locally?

assuming your cmake configuration includes -DCMAKE_EXPORT_COMPILE_COMMANDS=ON , run

run-clang-tidy -header-filter=../ -checks=clang-analyzer\* ../src/crwimage_int.cpp

[kevinbackhouse]

@kevinbackhouse I was right to question this get() usage. clang-analyzer complains with:

@neheb: which log file is this in? Or can you give me instructions to reproduce it locally?

assuming your cmake configuration includes -DCMAKE_EXPORT_COMPILE_COMMANDS=ON , run

run-clang-tidy -header-filter=../ -checks=clang-analyzer\* ../src/crwimage_int.cpp

@neheb: you already fixed it in f81fd33, right?

[neheb]

Yes

[kevinbackhouse]

I think this looks good now. In future though, please could you not bundle so many unrelated changes into one PR? I find smaller PRs easier to review. Also, if a change causes a regression, I find it's easier to track down what caused it if each individual PR is smaller.