several improvements to `Path::getAbsolutePath()` by firewave · Pull Request #6542 · cppcheck-opensource/cppcheck

firewave · 2024-06-20T22:19:02Z

fixed result on Linux if parts discarded by .. do not exist
small adjustments to Windows implementation to match Linux one
throw from getAbsolutePath() if the given path does not exist

firewave · 2024-06-21T13:30:47Z

This now crashes in Cygwin:

Thread 1 "testrunner" received signal SIGABRT, Aborted.
0x00007ff87dd6d5e4 in ntdll!ZwWaitForSingleObject ()
   from /cygdrive/c/WINDOWS/SYSTEM32/ntdll.dll
(gdb) bt
#0  0x00007ff87dd6d5e4 in ntdll!ZwWaitForSingleObject ()
   from /cygdrive/c/WINDOWS/SYSTEM32/ntdll.dll
#1  0x00007ff87b5b1c4e in WaitForSingleObjectEx ()
   from /cygdrive/c/WINDOWS/System32/KERNELBASE.dll
#2  0x00007ff801d8ba60 in sigfillset () from /usr/bin/cygwin1.dll
#3  0x00007ff801d8793b in sched_getscheduler () from /usr/bin/cygwin1.dll
#4  0x00007ff801d87d11 in sched_getscheduler () from /usr/bin/cygwin1.dll
#5  0x00007ff801ed71d8 in cygwin1!abort () from /usr/bin/cygwin1.dll
#6  0x000000057b1a548e in cygstdc++-6!_ZN9__gnu_cxx27__verbose_terminate_handlerEv () from /usr/bin/cygstdc++-6.dll
#7  0x000000057b19bb86 in cygstdc++-6!_ZN10__cxxabiv111__terminateEPFvvE ()
   from /usr/bin/cygstdc++-6.dll
#8  0x000000057b29da73 in cygstdc++-6!.cxa_call_terminate ()
   from /usr/bin/cygstdc++-6.dll
#9  0x000000057b19befc in cygstdc++-6!_ZN10__cxxabiv129__pointer_to_member_type_infoD2Ev () from /usr/bin/cygstdc++-6.dll
#10 0x000000050caacbc5 in cyggcc_s-seh-1!_GCC_specific_handler ()
   from /usr/bin/cyggcc_s-seh-1.dll
#11 0x000000057b29e955 in cygstdc++-6!.gxx_personality_seh0 ()
   from /usr/bin/cygstdc++-6.dll
#12 0x00007ff87dd7292f in ntdll!.chkstk ()
   from /cygdrive/c/WINDOWS/SYSTEM32/ntdll.dll
#13 0x00007ff87dd22554 in ntdll!RtlRaiseException ()
   from /cygdrive/c/WINDOWS/SYSTEM32/ntdll.dll
#14 0x00007ff87dd7143e in ntdll!KiUserExceptionDispatcher ()
   from /cygdrive/c/WINDOWS/SYSTEM32/ntdll.dll
#15 0x00007ff87b5bba99 in RaiseException ()
   from /cygdrive/c/WINDOWS/System32/KERNELBASE.dll
#16 0x000000050caacca1 in cyggcc_s-seh-1!_Unwind_RaiseException ()
   from /usr/bin/cyggcc_s-seh-1.dll
#17 0x000000057b29e0fb in cygstdc++-6!.cxa_throw ()
   from /usr/bin/cygstdc++-6.dll
#18 0x000000057b29f793 in cygstdc++-6!_ZSt20__throw_system_errori ()
   from /usr/bin/cygstdc++-6.dll
#19 0x000000010107175b in std::mutex::lock (
    this=0x101603150 <realFilePathMap+48>)
    at /usr/lib/gcc/x86_64-pc-cygwin/11/include/c++/bits/std_mutex.h:104
#20 0x0000000100ff9dba in std::lock_guard<std::mutex>::lock_guard (
    this=0x7ffffb990, __m=...)
    at /usr/lib/gcc/x86_64-pc-cygwin/11/include/c++/bits/std_mutex.h:229
#21 0x000000010091770b in RealFileNameMap::getCacheEntry (
    this=0x101603120 <realFilePathMap>, path=..., returnPath=...)
    at externals/simplecpp/simplecpp.cpp:2362
#22 0x00000001004077a5 in realFilename (f=...)
    at externals/simplecpp/simplecpp.cpp:2430
#23 0x0000000100407f08 in simplecpp::simplifyPath (path=...)
    at externals/simplecpp/simplecpp.cpp:2565
#24 0x000000010066470a in Path::simplifyPath (originalPath=...)
    at lib/path.cpp:91
#25 0x0000000100665974 in Path::getAbsoluteFilePath (filePath=...)
    at lib/path.cpp:372
#26 0x000000010065061e in Library::load (this=0x7ffffc338,
    exename=0x57b2a25b8 <cygstdc++-6!_ZNSs4_Rep20_S_empty_rep_storageE+24> "",
    path=0x101362178 <__gnu_cxx::__default_lock_policy+68> "std.cfg",
    debug=false) at lib/library.cpp:139
#27 0x000000010041ad9f in TestFixture::SettingsBuilder::library (
    this=0x7ffffc1b0,
    lib=0x101362178 <__gnu_cxx::__default_lock_policy+68> "std.cfg")
    at test/fixture.cpp:458
#28 0x0000000100914fc5 in TestValueFlow::TestValueFlow (
    this=0x101662840 <(anonymous namespace)::instance_TestValueFlow>)
    at test/testvalueflow.cpp:42
#29 0x000000010042c6f1 in __static_initialization_and_destruction_0 (
    __initialize_p=1, __priority=65535) at test/testvalueflow.cpp:8566
#30 0x000000010042c72f in _GLOBAL__sub_I_testvalueflow.cpp(void) ()
    at test/testvalueflow.cpp:8566
#31 0x00007ff801d1664d in cygwin1!.main () from /usr/bin/cygwin1.dll
#32 0x000000010041e1d3 in main (argc=1, argv=0xa00001b30) at test/main.cpp:27

firewave · 2024-06-21T13:34:48Z

Looks like it is caused by the static initialization order. So this requires https://trac.cppcheck.net/ticket/12080 to be fixed first.

firewave · 2024-07-01T08:12:56Z

We could also make the cache in simplecpp optional.

Nevertheless these caches are completely horrible and need to be implemented in a different way.

firewave · 2024-08-14T17:47:15Z

The crash is fixed because the test fixtures are no longer global objects which depend on the initialization order.

firewave · 2024-08-15T11:57:03Z

        absolute_path = absolute;
    free(absolute);
+    if (!filePath.empty() && !spath.empty() && absolute_path.empty() && !exists(spath))
+        throw std::runtime_error("path '" + filePath + "' does not exist");


This makes sure we do not get unexpected results. This needs an integration test which triggers this though (I am hoping one of the existing ones will fail so I can derive from that).

The only case where this could happen is in loadVisualStudioProperties() and that code looks quite shifty and need some proper testing. As Path::isAbsolute() also needs some work that will probably happen in that context.

firewave · 2024-09-18T10:20:33Z

We could also make the cache in simplecpp optional.

Nevertheless these caches are completely horrible and need to be implemented in a different way.

I filed cppcheck-opensource/simplecpp#361 upstream.

danmar · 2024-09-28T15:05:08Z

    char absolute[_MAX_PATH];
    if (_fullpath(absolute, filePath.c_str(), _MAX_PATH))
        absolute_path = absolute;
+    if (absolute_path.back() == '\\')


there is a good chance that absolute_path is empty right? don't call back() in that case.

That would indeed be undefined: https://en.cppreference.com/w/cpp/string/basic_string/back - and we even report that. I wonder if that might exists in other parts of the code. We could probably be more aggressive about detecting that - maybe similar to a NULL dereference.

The _fullpath() call might need better errorhandling.

That would indeed be undefined: https://en.cppreference.com/w/cpp/string/basic_string/back - and we even report that. I wonder if that might exists in other parts of the code. We could probably be more aggressive about detecting that - maybe similar to a NULL dereference.

This is not reported because it is in the _WIN32 code which we do not analyze in the CI. We should probably add analysis steps for files using those to the selfchecks.

Fixed. I also added a bunch of missing test cases and TODOs.

I also filed https://trac.cppcheck.net/ticket/13158 about enhancing the selfcheck.

danmar · 2024-10-03T11:39:52Z

    if (absolute)
        absolute_path = absolute;
    free(absolute);
+    if (!filePath.empty() && !spath.empty() && absolute_path.empty() && !exists(spath))


we know that filePath is not empty. so condition !filePath.empty() && is redundant.
I am not sure shouldn't we use || instead of && here. each of those conditions look bad?

Even though there is a preprocessor check that should be detected.

I will review the remaining conditions and add a comment about the intention.

I filed https://trac.cppcheck.net/ticket/13178 about detecting this.

I adjusted the check and added a comment.

…ed by `..` do not exist

… to match Linux one

This comment was marked as outdated.

Sign in to view

firewave marked this pull request as draft June 20, 2024 23:48

firewave mentioned this pull request Jul 22, 2024

Re-evaluate cache implementations cppcheck-opensource/simplecpp#361

Open

firewave force-pushed the abspath-test branch 2 times, most recently from 71ef47a to 49daf24 Compare August 14, 2024 17:46

firewave force-pushed the abspath-test branch from 49daf24 to b62a0d9 Compare August 15, 2024 11:52

firewave changed the title ~~small improvements to Path::getAbsolutePath()~~ several improvements to Path::getAbsolutePath() Aug 15, 2024

firewave commented Aug 15, 2024

View reviewed changes

Comment thread lib/path.h

firewave force-pushed the abspath-test branch from b62a0d9 to fd971fa Compare September 18, 2024 10:19

firewave marked this pull request as ready for review September 18, 2024 10:19

danmar reviewed Sep 28, 2024

View reviewed changes

firewave marked this pull request as draft September 29, 2024 00:00

firewave force-pushed the abspath-test branch from fd971fa to 290f233 Compare September 30, 2024 15:55

firewave marked this pull request as ready for review September 30, 2024 16:06

danmar reviewed Oct 3, 2024

View reviewed changes

firewave marked this pull request as draft October 4, 2024 12:24

firewave added 3 commits November 14, 2024 22:25

Path: fixed result from getAbsolutePath() on Linux if parts discard…

b043029

…ed by `..` do not exist

Path: small adjustments to getAbsolutePath() Windows implementation…

eb4ec20

… to match Linux one

Path: throw from getAbsolutePath() if the given path does not exist

fb1ddd7

firewave force-pushed the abspath-test branch from 290f233 to fb1ddd7 Compare November 14, 2024 21:29

firewave marked this pull request as ready for review November 14, 2024 21:29

danmar approved these changes Dec 4, 2024

View reviewed changes

firewave merged commit 640f82d into cppcheck-opensource:main Dec 4, 2024

firewave deleted the abspath-test branch December 4, 2024 22:42

Conversation

firewave commented Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

firewave commented Jun 21, 2024

Uh oh!

firewave commented Jun 21, 2024

Uh oh!

firewave commented Jul 1, 2024

Uh oh!

firewave commented Aug 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

firewave commented Sep 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

firewave commented Jun 20, 2024 •

edited

Loading