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

[Slava Gurevich via Phabricator via lldb-commits]

fixathon added a comment.

Added inline comments to clarify the fixes. Specifically need feedback on the null-termination of strings since it seems possible this may not be needed based on existing comments.



================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:551-552
       case FPURegSet: {
-        uint8_t *fpu_reg_buf = (uint8_t *)&fpu.floats.s[0];
+        uint8_t *fpu_reg_buf = (uint8_t *)&fpu.floats;
         const int fpu_reg_buf_size = sizeof(fpu.floats);
         if (data.ExtractBytes(offset, fpu_reg_buf_size, eByteOrderLittle,
----------------
**fpu_reg_buf** is the destination buffer, and **fpu_reg_buf_size** is the number of bytes to write.





Originally, the buffer had the address of fpu.floats.s[0] that is the start of a 128 bytes long array, and fpu_reg_buf_size is 256 bytes. However, there's no buffer overrun because the target buffer is a union member, and the union has sufficient size:

 struct FPU {
    union {
      uint32_t s[32];                            /// 4*32 =  128 bytes
      uint64_t d[32];                            /// 8*32  = 256 bytes
      QReg q[16]; // the 128-bit NEON registers  /// 16*16 = 256 bytes
    } floats;
    uint32_t fpscr;
  };

Instead we have a misleading use of the smaller of the available buffers inside of the union to obtain the buffer address. According to the C++ specification, all non-static union members of the union, as well as the union itself will have the same address in memory. I have modified the code to be less misleading, and to keep the static code inspection happy, while the outcome will be exactly the same as before. 


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:4125
+                symbol_name +
+                ((symbol_name && (symbol_name[0] == '_')) ? 1 : 0)));
           } else
----------------
potential null dereference


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


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:6735-6736
           memset(namebuf, 0, sizeof(namebuf));
           // this is the uncommon case where strncpy is exactly
           // the right one, doesn't need to be nul terminated.
           strncpy(namebuf, lcnote->name.c_str(), sizeof(namebuf));
----------------
Please clarify why this is the case. Thanks


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


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:6900
 
         offset = entries_fileoff;
         for (uint32_t i = 0; i < imgcount; i++) {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131554