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

Revert "Linux: Make the C++ code compatible with old compilers..." #1377

Closed
wants to merge 1 commit into from

Conversation

Jertzukka
Copy link
Contributor

@Jertzukka Jertzukka commented Jul 10, 2024

This reverts commit 9697416.

This commit breaks admin prompts on Linux causing a segmentation fault for example when mounting a volume.

Tested on Ubuntu 23.10.

#0  0x0000555555769f61 in VeraCrypt::AdminPasswordRequestHandler::operator()(std::basic_string<char, std::char_traits<char>, std::allocator<char> >&) ()
#1  0x0000555555896944 in std::unique_ptr<VeraCrypt::MountVolumeResponse, std::default_delete<VeraCrypt::MountVolumeResponse> > VeraCrypt::CoreService::SendRequest<VeraCrypt::MountVolumeResponse>(VeraCrypt::CoreServiceRequest&) ()
#2  0x000055555588fac4 in VeraCrypt::CoreService::RequestMountVolume(VeraCrypt::MountOptions&) ()
#3  0x00005555558b4bda in VeraCrypt::CoreServiceProxy<VeraCrypt::CoreLinux>::MountVolume(VeraCrypt::MountOptions&) ()
#4  0x00005555557becf4 in VeraCrypt::MountThreadRoutine::ExecutionCode() ()
#5  0x000055555587a923 in VeraCrypt::WaitThreadRoutine::Execute() ()
#6  0x0000555555877c3b in VeraCrypt::WaitThread::Entry() ()
#7  0x0000555555c866ee in wxThreadInternal::PthreadStart(wxThread*) ()
#8  0x00007ffff6897b5a in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:444
        ret = <optimized out>
        pd = <optimized out>
        out = <optimized out>
        unwind_buf = {cancel_jmp_buf = {{jmp_buf = {140737329592432, 6416269201991415161, -152, 11, 140737488333712, 140737010200576, -6416242813289482887, -6416283895114877575}, mask_was_saved = 0}}, priv = {pad = {0x0, 0x0, 0x0, 0x0}, data = {prev = 0x0, cleanup = 0x0, canceltype = 0}}}
        not_first_call = <optimized out>
#9  0x00007ffff69285fc in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:78

Also I don't know how valuable it is to provide support for so old versions. When I was attempting to setup a VM with CentOS 6 for building, I had to give up after a bit as even the package repos will tell you "This directory (and version of CentOS) is deprecated" and "The whole CentOS 6 is dead and shouldn't be used anywhere at all". These changes are really hard to test as even setting up an environment is such a hassle.

The commit should probably be reverted before the relevant bits are refactored into working order, if that is still your goal.

….4.7 on CentOS 6)"

This reverts commit 9697416.

Breaks admin prompts on Linux.
@idrassi
Copy link
Member

idrassi commented Jul 10, 2024

Thank you for the report and sorry again for this fail.
I'm willing to keep these changes because build on CentOS 6 is still needed for creating a generic binary.
I analyzed the issue and it is solved if I just move declaration of AdminPasswordRequestHandler inside GraphicUserInterface::GetAdminPasswordRequestHandler again.

I don't understand yet why the location of the definition of AdminPasswordRequestHandler matters since its implementation doesn't depend on elements of the scope.

I will move back AdminPasswordRequestHandler to its initial location and I will try to figure out why such issue exist in the first place.

@Jertzukka
Copy link
Contributor Author

Jertzukka commented Jul 10, 2024

Changes in 1312c53 and 526f031 seems to fix the segfault, closing.

@Jertzukka Jertzukka closed this Jul 10, 2024
@Jertzukka
Copy link
Contributor Author

@idrassi Still causes segfaults on CLI with certain prompts, such as veracrypt --text --non-interactive /path/to/container.hc. Probably needs reverts also on TextUserInterface.cpp and/or UserInterface.cpp

Thread 1 "veracrypt" received signal SIGSEGV, Segmentation fault.
0x0000555555769f61 in VeraCrypt::AdminPasswordRequestHandler::operator()(std::basic_string<char, std::char_traits<char>, std::allocator<char> >&) ()
(gdb) bt full
#0  0x0000555555769f61 in VeraCrypt::AdminPasswordRequestHandler::operator()(std::basic_string<char, std::char_traits<char>, std::allocator<char> >&) ()
#1  0x00005555558971e6 in std::unique_ptr<VeraCrypt::MountVolumeResponse, std::default_delete<VeraCrypt::MountVolumeResponse> > VeraCrypt::CoreService::SendRequest<VeraCrypt::MountVolumeResponse>(VeraCrypt::CoreServiceRequest&) ()
#2  0x0000555555890366 in VeraCrypt::CoreService::RequestMountVolume(VeraCrypt::MountOptions&) ()
#3  0x00005555558b547c in VeraCrypt::CoreServiceProxy<VeraCrypt::CoreLinux>::MountVolume(VeraCrypt::MountOptions&) ()
#4  0x0000555555784553 in VeraCrypt::UserInterface::ProcessCommandLine() ()
#5  0x0000555555762cad in VeraCrypt::TextUserInterface::OnRun() ()
#6  0x0000555555bfd7e8 in wxEntry(int&, wchar_t**) ()
#7  0x00005555557a8042 in main ()

@idrassi
Copy link
Member

idrassi commented Jul 13, 2024

@Jertzukka Thanks for the report. I analyzed this crash and, as you can see in the gdb trace below, it occurs because the UI member variable of AdminPasswordRequestHandler is NULL during the call, which is absurd since we are setting it correctly in the constructor.

I added a trace in the constructor and, actually, it is never called! I could not believe my eyes... The constructor is definitely called in TextUserInterface::GetAdminPasswordRequestHandler, so what is happening?

Then I noticed something: the file UserInterface.cpp also contains the definition of a class AdminPasswordRequestHandler in the VeraCrypt namespace, and we are creating an instance of this class directly in UserInterface::Init without calling GetAdminPasswordRequestHandler!!

Now it is clear: there is a type confusion that makes the code create an instance with one class definition and then use the operator() of the other class definition. Why didn't the linker complain about this?

Anyway, I have pushed a fix for this, and now there should not be any more crashes: 1ee93df

What a ride!

VeraCrypt::AdminPasswordRequestHandler::operator() (this=0x555555c7a350, passwordStr="") at TextUserInterface.cpp:39
39				UI->ShowString (_("Enter your user password or administrator password: "));
(gdb) bt
#0  VeraCrypt::AdminPasswordRequestHandler::operator()(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)
    (this=0x555555c7a350, passwordStr="") at TextUserInterface.cpp:39

(gdb) frame 0
#0  VeraCrypt::AdminPasswordRequestHandler::operator() (this=0x555555c7a350, passwordStr="") at TextUserInterface.cpp:39
39				UI->ShowString (_("Enter your user password or administrator password: "));
(gdb) print UI
$1 = (VeraCrypt::TextUserInterface *) 0x0
(gdb) 

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.

2 participants