Skip to content

Fix a crash from character type confusion interaction with libedit #75388

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 14, 2023
Merged

Fix a crash from character type confusion interaction with libedit #75388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lldb/include/lldb/Host/Editline.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ using EditLineCharType = char;
// to wchar_t. It is not possible to detect differentiate between the two
// versions exactly, but this is a pretty good approximation and allows us to
// build against almost any editline version out there.
// It does, however, require extra care when invoking el_getc, as the type
// of the input is a single char buffer, but the callback will write a wchar_t.
#if LLDB_EDITLINE_USE_WCHAR || defined(EL_CLIENTDATA) || LLDB_HAVE_EL_RFUNC_T
using EditLineGetCharType = wchar_t;
#else
Expand Down
10 changes: 8 additions & 2 deletions lldb/source/Host/common/Editline.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -978,8 +978,14 @@ void Editline::DisplayCompletions(
break;

fprintf(editline.m_output_file, "More (Y/n/a): ");
char reply = 'n';
int got_char = el_getc(editline.m_editline, &reply);
// The type for the output and the type for the parameter are different,
// to allow interoperability with older versions of libedit. The container
// for the reply must be as wide as what our implementation is using,
// but libedit may use a narrower type depending on the build
// configuration.
EditLineGetCharType reply = L'n';
int got_char = el_wgetc(editline.m_editline,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this use el_wgetc instead of us switching based on LLDB_EDITLINE_USE_WCHAR? I'm not too familiar with editline, but it seems strange to me that we would use el_wgetc even if there's no wchar_t support.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

el_wgetc is #defined to el_getc if there's no wchar_t support (it's not at all consistent in this file, but I wanted to targeted fix for the crash). The core problem is actually with the #if code below the comment in the header:

#if LLDB_EDITLINE_USE_WCHAR || defined(EL_CLIENTDATA) || LLDB_HAVE_EL_RFUNC_T
using EditLineGetCharType = wchar_t;
#else
using EditLineGetCharType = char;
#endif

We set EditLineGetCharType to wchar_t, even if you are building with LLDB_EDITLINE_USE_WCHAR=0 with newer versions of libedit (the ones that define EL_CLIENTDATA) so the function that reads a character (called from within libedit) is always reading a wchar_t and writing it to the buffer that's backed by &reply.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized I didn't point you at the code to answer your simple question: At the top of EditLine.cpp, all the el_w* functions are #defined to el_* functions if you're build without LLDB_EDITLINE_USE_WCHAR

reinterpret_cast<EditLineCharType *>(&reply));
// Check for a ^C or other interruption.
if (editline.m_editor_status == EditorStatus::Interrupted) {
editline.m_editor_status = EditorStatus::Editing;
Expand Down