Skip to content

[libunwind] Remove unnecessary strcpy dependency #72043

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
Nov 15, 2023
Merged

[libunwind] Remove unnecessary strcpy dependency #72043

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: 1 addition & 1 deletion libunwind/src/UnwindLevel1-gcc-ext.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ _Unwind_Backtrace(_Unwind_Trace_Fn callback, void *ref) {
// Create a mock exception object for force unwinding.
_Unwind_Exception ex;
memset(&ex, '\0', sizeof(ex));
strcpy((char *)&ex.exception_class, "CLNGUNW");
memcpy(&ex.exception_class, "CLNGUNW", sizeof(ex.exception_class));
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use the string length here instead of the size of the exception class?

Copy link
Contributor Author

@michael-kenzel michael-kenzel Nov 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

My reasoning was that if the string happened to be too long for some reason, we'd want to ensure that we don't write past the member. Using the string length would require duplicating the string literal, or introducing a constant, which would probably require a macro since this is C and so we can't just use constexpr (I doubt we can assume C23?) or some helper function/macro to wrap the copy. Either option, using the string length or member length, has the downside of not considering the other length. Ideally, we'd have a _Static_assert to ensure the sizes match. But that then puts us back to the problem of needing to duplicate the string literal. I couldn't find a way to put any such improvements into plain C without making this code significantly more complex. I was also not quite sure what versions of C this needs to build under, which would have implications regarding what features can be used.

One very simple solution (at least code-wise) would be to just do

  _Unwind_Exception ex = {
    .exception_class = 0x574E55474E4C43  // "CLNGUNW"
  };

That would take care of both the memset and the memcpy. But this then assumes little endian and so would be a potential ABI break on big-endian systems?

Overall, I think it's reasonable to assume that this string isn't just gonna change in a way that would make it not match the size of the member anymore. The current implementation was already broken in that case as well (at least if the string would happen to be longer). Unfortunately, no compiler seems to issue a warning if you make the string not match the size of the member.

Copy link
Member

Choose a reason for hiding this comment

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

I agree this is fine as is. Assigning the integer directly will cause endian issues, so using memcpy is better.

If we want to be extra safe, we could #define the string value and add a static_assert().

#endif

// walk each frame
Expand Down
2 changes: 1 addition & 1 deletion libunwind/test/forceunwind.pass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ __attribute__((noinline)) void foo() {
#if defined(_LIBUNWIND_ARM_EHABI)
// Create a mock exception object.
memset(e, '\0', sizeof(*e));
strcpy(reinterpret_cast<char *>(&e->exception_class), "CLNGUNW");
memcpy(&e->exception_class, "CLNGUNW", sizeof(e->exception_class));
#endif
_Unwind_ForcedUnwind(e, stop, (void *)&foo);
}
Expand Down