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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
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);
----------------



================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:541
+             count > 0 && count <= sizeof(gpr.r) && i < count - 1; ++i) {
           gpr.r[i] = data.GetU32(&offset);
         }
----------------
Just a quick FYI: if the "offset" value ends up being out of bounds for the data, it will stop parsing anything and set the offset value to UINT64_MAX, so this is safe already.




================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:4116
           const char *reexport_name_cstr = strtab_data.PeekCStr(nlist.n_value);
           if (reexport_name_cstr && reexport_name_cstr[0]) {
             type = eSymbolTypeReExported;
----------------



================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:4125
+                symbol_name +
+                ((symbol_name && (symbol_name[0] == '_')) ? 1 : 0)));
           } else
----------------
fixathon wrote:
> potential null dereference
Revert, see above code suggestion.


================
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.
Need to revert this. If the segment name is actually 16 characters long, you will change the segment name which we can't do and there is no NULL termination in this case.


================
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));
----------------
jasonmolenda wrote:
> 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.
Revert this. If the name is actually 16 characters long it isn't supposed to be null terminated.


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:6900
 
         offset = entries_fileoff;
         for (uint32_t i = 0; i < imgcount; i++) {
----------------
fixathon wrote:
> offset is re-assigned here, so there's no point in the previous 2 lines. However it is likely those follow the decoding format specification, so left it in as comments for clarity.
Maybe change to something like:
```
// entries_size is not used, nor is the unused entry.
// uint32_t entries_size = m_data.GetU32(&offset);
// uint32_t unused = m_data.GetU32(&offset);
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131554