ipmi.c: Fix off by 1 memset

tasleson commented 9 months ago

The code was using the size of an element + number of elements for the memset which is incorrect. It should be the size of an element * the number of elements. However, as we are simply clearing memory for an array on the stack, a simple sizeof array will give us what we need, which is the number of bytes the array occupies.

This was discovered as a SIGABORT on Fedora 38 testing of pre-released compiled library package.

Note: replaced memsets for structs with struct initialization syntax as requested in review comments.

*** buffer overflow detected ***: terminated

Program received signal SIGABRT, Aborted.
0x00007ffff7e0a884 in __pthread_kill_implementation () from /lib64/libc.so.6
Missing separate debuginfos, use: dnf debuginfo-install glibc-2.37-14.fc38.x86_64
(gdb) bt
#0  0x00007ffff7e0a884 in __pthread_kill_implementation () from /lib64/libc.so.6
#1  0x00007ffff7db9afe in raise () from /lib64/libc.so.6
#2  0x00007ffff7da287f in abort () from /lib64/libc.so.6
#3  0x00007ffff7da360f in __libc_message.cold () from /lib64/libc.so.6
#4  0x00007ffff7e9e969 in __fortify_fail () from /lib64/libc.so.6
#5  0x00007ffff7e9d1a4 in __chk_fail () from /lib64/libc.so.6
#6  0x00007ffff7fb7789 in memset (__len=22, __ch=0, __dest=0x7fffffffaf60) at /usr/include/bits/string_fortified.h:59
#7  ipmicmd (ctx=ctx@entry=0x4052a0, sa=sa@entry=32, lun=lun@entry=0, netfn=netfn@entry=6, cmd=cmd@entry=89, datalen=datalen@entry=4, data=<optimized out>, resplen=<optimized out>, rlen=<optimized out>, resp=<optimized out>) at /home/tasleson/ledmon/src/lib/ipmi.c:82
#8  0x00007ffff7fb7b3d in get_dell_server_type (ctx=ctx@entry=0x4052a0) at /home/tasleson/ledmon/src/lib/dellssd.c:119
#9  0x00007ffff7fb7f07 in _is_dellssd_cntrl (ctx=0x4052a0, path=0x7fffffffb280 "/sys/devices/pci0000:00/0000:00:02.0/0000:0c:00.0") at /home/tasleson/ledmon/src/lib/cntrl.c:148
#10 _get_type (ctx=0x4052a0, path=0x7fffffffb280 "/sys/devices/pci0000:00/0000:00:02.0/0000:0c:00.0") at /home/tasleson/ledmon/src/lib/cntrl.c:218
#11 cntrl_device_init (ctx=0x4052a0, path=0x7fffffffb280 "/sys/devices/pci0000:00/0000:00:02.0/0000:0c:00.0") at /home/tasleson/ledmon/src/lib/cntrl.c:387
#12 _cntrl_add (path=0x7fffffffb280 "/sys/devices/pci0000:00/0000:00:02.0/0000:0c:00.0", ctx=0x4052a0) at /home/tasleson/ledmon/src/lib/sysfs.c:284
#13 _check_cntrl (ctx=ctx@entry=0x4052a0, path=<optimized out>) at /home/tasleson/ledmon/src/lib/sysfs.c:327
#14 0x00007ffff7fb9d49 in _scan_cntrl (ctx=0x4052a0) at /home/tasleson/ledmon/src/lib/sysfs.c:370
#15 sysfs_scan (ctx=0x4052a0) at /home/tasleson/ledmon/src/lib/sysfs.c:592
#16 led_scan (ctx=0x4052a0) at /home/tasleson/ledmon/src/lib/libled.c:96
#17 0x000000000040129b in main () at list_slots.c:29

tasleson commented 9 months ago

I'm not sure why the code checking requires sizeof(name) when sizeof is a unary operator, but I updated that to make the checker happy.

tasleson commented 9 months ago

Forgot to mention

ipmi.c: In function 'ipmicmd':
ipmi.c:71:38: error: variable-sized object may not be initialized except with an empty initializer
   71 |         uint8_t tresp[resplen + 1] = {0};

tasleson commented 9 months ago

The other thing that is perhaps not obvious from what I've stated thus far, but when we package things for fedora the packaging specifies addition compiler and linker flags by default which caused the SIGABORT and subsequently found this issue where we are zeroing 1 more byte on the stack than we should be.

intel / ledmon

ipmi.c: Fix off by 1 memset #194