[LLDB][NFC] Reliability fixes for ObjectFileMachO.cpp

[fixathon]

Summary

Static code inspection guided fixes for the following issues:

• dead code
• buffer not null-terminated
• null-dereference
• out-of-bounds access

[fixathon]

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.

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

552–553

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.

4126

potential null dereference

6351

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.

6740–6743

Please clarify why this is the case. Thanks

6744–6745

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.

6906

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.

[fixathon]

ping

[jasonmolenda]

Looks good to me with a few suggestions inlined.

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

536

'constraining'

539

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.

6351

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.

6744–6745

Same thing here - LC_NOTE name field is char[16] and is not guaranteed to be nul terminated.

[jasonmolenda]

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

539

FWIW the Aarch64 version of this function hardcodes the number of elements (where each general purpose register is 8-bytes, so count==2, and then there's one cpsr 4-byte register),

// x0-x29 + fp + lr + sp + pc (== 33 64-bit registers) plus cpsr (1
// 32-bit register)
if (count >= (33 * 2) + 1) {
  for (uint32_t i = 0; i < 29; ++i)
    gpr.x[i] = data.GetU64(&offset);
  gpr.fp = data.GetU64(&offset);
  gpr.lr = data.GetU64(&offset);
  gpr.sp = data.GetU64(&offset);
  gpr.pc = data.GetU64(&offset);
  gpr.cpsr = data.GetU32(&offset);

[fixathon]

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

539

Good catch. I'll: count <= sizeof(gpr.r)/sizeof(gpr.r[0])

[fixathon]

Addressing reviewer comments

[JDevlieghere]

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

6344

Nit: capital

[jasonmolenda]

LGTM. fwiw it's nul-terminated ("NUL" being the '\0' character), not null-terminated ((void*)0). i don't really care, but nb.

[fixathon]

Fix some nits in comments

[clayborg]

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

536–542

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.

539

4117

4126

Revert, see above code suggestion.

6351

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.

6744–6745

Revert this. If the name is actually 16 characters long it isn't supposed to be null terminated.

6906

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

[fixathon]

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

539

"count > 0 && count <= sizeof(gpr.r)/sizeof(uint32_t) && i < count - 1; ++i)" was causing a compiler warning, which is the reason I moved count out of the for loop condition

[fixathon]

Replied to comments, and adding the suggested modifications in https://reviews.llvm.org/D131743 because this diff has already been pushed.

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

while (!done) {
       int flavor = data.GetU32(&offset);
       uint32_t count = data.GetU32(&offset);
       lldb::offset_t next_thread_state = offset + (count * 4);
       switch (flavor) {
       case GPRAltRegSet:
       case GPRRegSet:
         // On ARM, the CPSR register is also included in the count but it is
         // not included in gpr.r so loop until (count-1).
+
+        // Prevent static analysis warnings by explicitly contstraining 'count'
+        // to acceptable range. Handle possible underflow of count-1
+        if (count > 0 && count <= sizeof(gpr.r) / sizeof(gpr.r[0])) {
           for (uint32_t i = 0; i < (count - 1); ++i) {
             gpr.r[i] = data.GetU32(&offset);
           }
+        }
         // Save cpsr explicitly.
         gpr.cpsr = data.GetU32(&offset);
 
         SetError(GPRRegSet, Read, 0);
         offset = next_thread_state;
         break;
 
       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) == fpu_reg_buf_size) {
           offset += fpu_reg_buf_size;
           fpu.fpscr = data.GetU32(&offset);
           SetError(FPURegSet, Read, 0);
         } else {
           done = true;
         }
auto ParseSymbolLambda = [&](struct nlist_64 &nlist, uint32_t nlist_idx,
         }
       } else {
         uint8_t n_type = N_TYPE & nlist.n_type;
         sym[sym_idx].SetExternal((N_EXT & nlist.n_type) != 0);
 
         switch (n_type) {
         case N_INDR: {
           const char *reexport_name_cstr = strtab_data.PeekCStr(nlist.n_value);
           if (reexport_name_cstr && reexport_name_cstr[0]) {
             type = eSymbolTypeReExported;
             ConstString reexport_name(reexport_name_cstr +
                                       ((reexport_name_cstr[0] == '_') ? 1 : 0));
             sym[sym_idx].SetReExportedSymbolName(reexport_name);
             set_value = false;
             reexport_shlib_needs_fixup[sym_idx] = reexport_name;
-            indirect_symbol_names.insert(
-                ConstString(symbol_name + ((symbol_name[0] == '_') ? 1 : 0)));
+            indirect_symbol_names.insert(ConstString(
+                symbol_name +
+                ((symbol_name && (symbol_name[0] == '_')) ? 1 : 0)));
           } else
             type = eSymbolTypeUndefined;
         } break;
 
         case N_UNDF:
           if (symbol_name && symbol_name[0]) {
             ConstString undefined_name(symbol_name +
                                        ((symbol_name[0] == '_') ? 1 : 0));
for (size_t i = 0; i < modules_count; i++) {
     for (size_t j = 0; j < sections_count; j++) {
       SectionSP section = sections->GetSectionAtIndex(j);
       if (!section->GetParent().get()) {
         addr_t vmaddr = section->GetLoadBaseAddress(&target);
         if (vmaddr == LLDB_INVALID_ADDRESS)
           continue;
         ConstString name = section->GetName();
         segment_vmaddr seg_vmaddr;
+        // This is the uncommon case where strncpy is exactly
+        // the right one, doesn't need to be nul terminated.
+        // 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.
         strncpy(seg_vmaddr.segname, name.AsCString(),
                 sizeof(seg_vmaddr.segname));
         seg_vmaddr.vmaddr = vmaddr;
         seg_vmaddr.unused = 0;
         segment_vmaddrs.push_back(seg_vmaddr);
       }
     }
     modules_segment_vmaddrs.push_back(segment_vmaddrs);
   }
 
   offset_t size_of_vmaddr_structs = 0;
if (make_core) {
 
         // Add LC_NOTE load commands
         for (auto &lcnote : lc_notes) {
           // Add the LC_NOTE load command to the file.
           buffer.PutHex32(LC_NOTE);
           buffer.PutHex32(sizeof(llvm::MachO::note_command));
           char namebuf[16];
           memset(namebuf, 0, sizeof(namebuf));
-          // this is the uncommon case where strncpy is exactly
+          // This is the uncommon case where strncpy is exactly
           // the right one, doesn't need to be nul terminated.
+          // LC_NOTE name field is char[16] and is not guaranteed to be
+          // nul-terminated.
           strncpy(namebuf, lcnote->name.c_str(), sizeof(namebuf));
           buffer.PutRawBytes(namebuf, sizeof(namebuf));
           buffer.PutHex64(lcnote->payload_file_offset);
           buffer.PutHex64(lcnote->payload.GetSize());
         }
 
         // Align to 4096-byte page boundary for the LC_SEGMENTs.
         file_offset = llvm::alignTo(file_offset, 4096);
 
         for (auto &segment : segment_load_commands) {
if (lc.cmd == LC_NOTE) {
         offset = fileoff;
         // Read the struct all_image_infos_header.
         uint32_t version = m_data.GetU32(&offset);
         if (version != 1) {
           return image_infos;
         }
         uint32_t imgcount = m_data.GetU32(&offset);
         uint64_t entries_fileoff = m_data.GetU64(&offset);
+        /* leaving the following dead code as comments for spec documentation
             offset += 4; // uint32_t entries_size;
             offset += 4; // uint32_t unused;
+        */
 
         offset = entries_fileoff;
         for (uint32_t i = 0; i < imgcount; i++) {
           // Read the struct image_entry.
           offset_t filepath_offset = m_data.GetU64(&offset);
           uuid_t uuid;
           memcpy(&uuid, m_data.GetData(&offset, sizeof(uuid_t)),
                  sizeof(uuid_t));
           uint64_t load_address = m_data.GetU64(&offset);
           offset_t seg_addrs_offset = m_data.GetU64(&offset);