[Lldb-commits] [PATCH] D131554: [LLDB][NFC] Reliability fixes for ObjectFileMachO.cpp

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda accepted this revision.
jasonmolenda added a comment.
This revision is now accepted and ready to land.

Looks good to me with a few suggestions inlined.



================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:537
+
+        // Prevent static analysis warnings by explicitly contsraining 'count'
+        // to acceptable range. Handle possible underflow of count-1
----------------
'constraining'


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:540
+        for (uint32_t i = 0;
+             count > 0 && count <= sizeof(gpr.r) && i < count - 1; ++i) {
           gpr.r[i] = data.GetU32(&offset);
----------------
The `count` field for a Darwin register context is the number of 4-byte entries in the object - it's a trick the kernel API often use so they can add fields later and the kernel knows what version of the object the userland process is requesting when it asks for "flavor, size" in a `get_thread_state` call.  This Aarch32 register context is `struct GPR {uint32_t r[16]; uint32_t cpsr};` or count 17, but `sizeof(gpr.r)` is going to be 64.  We only want to loop for 16 entries.


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:6345
                 sizeof(seg_vmaddr.segname));
+        seg_vmaddr.segname[sizeof(seg_vmaddr.segname) - 1] = '\0';
         seg_vmaddr.vmaddr = vmaddr;
----------------
fixathon wrote:
> strncpy may not have null-termination. Please comment if you believe this should not be corrected. I'd like to at least add the comments with sufficient explanation if that is the case.
Yes, the segment name in a Mach-O LC_SEGMENT/LC_SEGMENT_64 is  char[16] and is not guaranteed to be nul terminated if all 16 characters are used.  This code shouldn't be changed - adding a comment to that effect would be fine though.


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:6737-6738
           // the right one, doesn't need to be nul terminated.
           strncpy(namebuf, lcnote->name.c_str(), sizeof(namebuf));
+          namebuf[sizeof(namebuf) - 1] = '\0';
           buffer.PutRawBytes(namebuf, sizeof(namebuf));
----------------
fixathon wrote:
> strncpy may not have null-termination. Please comment if you believe this should not be corrected. I'd like to at least add the comments with sufficient explanation if that is the case.
Same thing here - LC_NOTE name field is char[16] and is not guaranteed to be nul terminated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131554