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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
