[Lldb-commits] [PATCH] D74096: [lldb/API] Fix the dangling pointer issue in SBThread::GetStopDescription

[Med Ismail Bennani via Phabricator via lldb-commits]

mib abandoned this revision.
mib added a comment.

I'll merge this with D73303 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74096/new/

https://reviews.llvm.org/D74096



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74096: [lldb/API] Fix the dangling pointer issue in SBThread::GetStopDescription

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/API/SBThread.cpp:361-363
 stop_desc =
 exe_ctx.GetProcessPtr()->GetUnixSignals()->GetSignalAsCString(
 stop_info_sp->GetValue());

JDevlieghere wrote:
> friss wrote:
> > I don't think this is generally safe. Creating a `std::string` from a 
> > nullptr is undefined (and the previous test makes it look like this pointer 
> > could be null).
> `GetStopDescription` now returns a `std::string`, so it should be fine here?
nvm, I didn't see the variable got assigned again


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74096/new/

https://reviews.llvm.org/D74096



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74096: [lldb/API] Fix the dangling pointer issue in SBThread::GetStopDescription

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/API/SBThread.cpp:357
 stop_desc = wp_desc;
 stop_desc_len = sizeof(wp_desc); // Include the NULL byte for size
   } break;

This looks like some "optimization" to not have to compute the strlen below if 
the string is known. I dont't think we need this anymore with the description 
being a string? 



Comment at: lldb/source/API/SBThread.cpp:394
+  if (!stop_desc.empty()) {
 if (dst)
+  return ::snprintf(dst, dst_len, "%s", stop_desc.c_str()) +

You could simplify this 

```
if (!stop_desc.empty() && dst) {
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74096/new/

https://reviews.llvm.org/D74096



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74096: [lldb/API] Fix the dangling pointer issue in SBThread::GetStopDescription

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/API/SBThread.cpp:361-363
 stop_desc =
 exe_ctx.GetProcessPtr()->GetUnixSignals()->GetSignalAsCString(
 stop_info_sp->GetValue());

friss wrote:
> I don't think this is generally safe. Creating a `std::string` from a nullptr 
> is undefined (and the previous test makes it look like this pointer could be 
> null).
`GetStopDescription` now returns a `std::string`, so it should be fine here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74096/new/

https://reviews.llvm.org/D74096



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74096: [lldb/API] Fix the dangling pointer issue in SBThread::GetStopDescription

[Frederic Riss via Phabricator via lldb-commits]

friss added inline comments.



Comment at: lldb/source/API/SBThread.cpp:361-363
 stop_desc =
 exe_ctx.GetProcessPtr()->GetUnixSignals()->GetSignalAsCString(
 stop_info_sp->GetValue());

I don't think this is generally safe. Creating a `std::string` from a nullptr 
is undefined (and the previous test makes it look like this pointer could be 
null).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74096/new/

https://reviews.llvm.org/D74096



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74096: [lldb/API] Fix the dangling pointer issue in SBThread::GetStopDescription

[Med Ismail Bennani via Phabricator via lldb-commits]

mib created this revision.
mib added reviewers: friss, JDevlieghere.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Instead of creating a char pointer to hold the stop reason description,
the reason is stored in a std::string.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74096

Files:
  lldb/source/API/SBThread.cpp


Index: lldb/source/API/SBThread.cpp
===
--- lldb/source/API/SBThread.cpp
+++ lldb/source/API/SBThread.cpp
@@ -325,15 +325,14 @@
 
   StopInfoSP stop_info_sp = exe_ctx.GetThreadPtr()->GetStopInfo();
   if (stop_info_sp) {
-const char *stop_desc =
-exe_ctx.GetThreadPtr()->GetStopDescription().c_str();
-if (stop_desc) {
+std::string stop_desc = exe_ctx.GetThreadPtr()->GetStopDescription();
+if (!stop_desc.empty()) {
   if (dst)
-return ::snprintf(dst, dst_len, "%s", stop_desc);
+return ::snprintf(dst, dst_len, "%s", stop_desc.c_str());
   else {
 // NULL dst passed in, return the length needed to contain the
 // description
-return ::strlen(stop_desc) + 1; // Include the NULL byte for size
+return stop_desc.size() + 1; // Include the NULL byte for size
   }
 } else {
   size_t stop_desc_len = 0;
@@ -362,7 +361,7 @@
 stop_desc =
 exe_ctx.GetProcessPtr()->GetUnixSignals()->GetSignalAsCString(
 stop_info_sp->GetValue());
-if (stop_desc == nullptr || stop_desc[0] == '\0') {
+if (stop_desc.empty()) {
   static char signal_desc[] = "signal";
   stop_desc = signal_desc;
   stop_desc_len =
@@ -391,13 +390,13 @@
 break;
   }
 
-  if (stop_desc && stop_desc[0]) {
+  if (!stop_desc.empty()) {
 if (dst)
-  return ::snprintf(dst, dst_len, "%s", stop_desc) +
+  return ::snprintf(dst, dst_len, "%s", stop_desc.c_str()) +
  1; // Include the NULL byte
 
 if (stop_desc_len == 0)
-  stop_desc_len = ::strlen(stop_desc) + 1; // Include the NULL byte
+  stop_desc_len = stop_desc.size() + 1; // Include the NULL byte
 
 return stop_desc_len;
   }


Index: lldb/source/API/SBThread.cpp
===
--- lldb/source/API/SBThread.cpp
+++ lldb/source/API/SBThread.cpp
@@ -325,15 +325,14 @@
 
   StopInfoSP stop_info_sp = exe_ctx.GetThreadPtr()->GetStopInfo();
   if (stop_info_sp) {
-const char *stop_desc =
-exe_ctx.GetThreadPtr()->GetStopDescription().c_str();
-if (stop_desc) {
+std::string stop_desc = exe_ctx.GetThreadPtr()->GetStopDescription();
+if (!stop_desc.empty()) {
   if (dst)
-return ::snprintf(dst, dst_len, "%s", stop_desc);
+return ::snprintf(dst, dst_len, "%s", stop_desc.c_str());
   else {
 // NULL dst passed in, return the length needed to contain the
 // description
-return ::strlen(stop_desc) + 1; // Include the NULL byte for size
+return stop_desc.size() + 1; // Include the NULL byte for size
   }
 } else {
   size_t stop_desc_len = 0;
@@ -362,7 +361,7 @@
 stop_desc =
 exe_ctx.GetProcessPtr()->GetUnixSignals()->GetSignalAsCString(
 stop_info_sp->GetValue());
-if (stop_desc == nullptr || stop_desc[0] == '\0') {
+if (stop_desc.empty()) {
   static char signal_desc[] = "signal";
   stop_desc = signal_desc;
   stop_desc_len =
@@ -391,13 +390,13 @@
 break;
   }
 
-  if (stop_desc && stop_desc[0]) {
+  if (!stop_desc.empty()) {
 if (dst)
-  return ::snprintf(dst, dst_len, "%s", stop_desc) +
+  return ::snprintf(dst, dst_len, "%s", stop_desc.c_str()) +
  1; // Include the NULL byte
 
 if (stop_desc_len == 0)
-  stop_desc_len = ::strlen(stop_desc) + 1; // Include the NULL byte
+  stop_desc_len = stop_desc.size() + 1; // Include the NULL byte
 
 return stop_desc_len;
   }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits