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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits