Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ssh crash on reconnect #459

Merged

Conversation

ssvine
Copy link
Contributor

@ssvine ssvine commented Apr 15, 2024

This pull request fixes this external bug.

Finally, I found the root cause of the crash.

This crash happens because extern "C" function select_result throws exception but we use /EHsc flag, which leads to UB. Because of UB the compiler can create a code that calls a destructor of already destroyed or never created object while doing stack unwinding.

Also this pull request fixes freeing of FCallbackSet in FreeBackend function.

UPDATE:

@michaellukashov, I updated this pull request with another one that was previously created separately, because otherwise merging them would result in a error.

So, I describe that appended PR: "Include map file in releases and fix optimizations"

There are several issues with release builds:

  1. If NetBox crashes and Far is able to create a crash report, that report won't contain meaningful names of NetBox functions and a random user may create an issue attaching such a problematic report. So, it's a good idea to include rather small .map file in the binary distribution, because Far can use them to convert addresses to meaningful names. (Far releases include .map files for all executables.)
  2. Release build is not fully optimized.

This PR adds NetBox.map file to the release build.

It also fixes and simplifies compiler and linker flags among different build types:

  • Remove delay load libraries that are statically linked with Far - they are already loaded in the process address space.
  • Remove compiler / linker flags that are duplicated, set by default, redundant, or implied by other flags
  • Remove /GS- flag as security measure
  • Do not use DEBUG define for RelWithDebInfo
  • Make Release compile for the maximum speed, RelWithDebInfo - the same as Release but with .pdb and .map files, and MinSizeRel - the same as Release but compiled for the smallest size

ssvine added 2 commits April 15, 2024 06:32
This happens because extern "C" function select_result throws exception but we use /EHsc flag, which leads to UB. Because of that the compiler can create a code that calls a destructor of already destroyed or never created object while doing stack unwinding.
@ssvine ssvine force-pushed the fix-ssh-crash-on-reconnect branch from 6f07eda to e741c23 Compare April 16, 2024 20:21
ssvine added 4 commits April 22, 2024 19:06
1. Remove duplicates
2. Remove flags that are included by default
3. Fix:  NETBOX_DLL_LINK_FLAGS was not included in xxx_RELEASE and xxx_DEBUG
4. Make Release, RelWithDebInfo, and MinSizeRel configs use the same linker flags
1. Remove explicit /O1 and /O2, CMake adds them
2. Remove flags that are implied by /O1 and /O2 flags
3. Remove /Ob1, /Ob2 to redefine CMake's choice
4. Remove /GS- (as security measure)
5. Do not use DEBUG define for RelWithDebInfo
@ssvine ssvine force-pushed the fix-ssh-crash-on-reconnect branch from b61b362 to 4cc05b5 Compare April 22, 2024 16:07
Copy link

sonarcloud bot commented Apr 22, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@michaellukashov michaellukashov merged commit 58942b8 into michaellukashov:main May 5, 2024
2 checks passed
@ssvine ssvine deleted the fix-ssh-crash-on-reconnect branch May 5, 2024 14:25
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.

Crash in NetBox after some inactivity
2 participants