This review was made against lsvpd version 1.7.5-0ubuntu1; I think this codebase would benefit from ASAN, valgrind, and consistent use of Lindent or similar script around indent(1) or similar code formatter.
There is recurring "hiding" of errno and other specific errors that are replaced by less-useful generic errors or incorrect errors. This can lead to more difficult debugging of live systems. Sometimes potentially incorrect assumptions are drawn based solely on an error return without checking errno for potential remediation or accurate error messages. I recommend the entire codebase be examined with this in mind, it is so frequent. There's more issues: deleteList() while (!head) should be while (head) -- the current implementation never frees anything, NULL dereference and sigsegv if passed NULL __lsvpdInit() does not memset the struct sigaction sigact to zero ensureEnv() doesn't check if env is a directory, has expected ownership. has expected permissions; it returns 0 for most conditions and -1 only if the mkdir() fails. I suspect the function is entirely incorrect. SysFSTreeCollector::getDevTreePath() misses fgets() error check lsvpd_hexify() uses delete instead of delete[] hexify() uses delete instead of delete[] hexify() leaks ret if the input length is 0 device_scsi_sg_resp_len() returns uninitialized garbage if evpd isn't 0 or 1 RtasCollector::rtasGetVPD() may leak locCode via most branches of switch RtasCollector::rtasGetVPD() may leak list via error return RtasCollector::rtasGetVPD() does not check size += current->size; loop for overflow, is this a possibility? FSWalk::fs_getDirContents() does not validate length of path_t, nor files in the directory, before copying their names into a fixed size buffer; probably the whole function should be re-written to use C++ strings instead device_open() uses incorrect <= 0 error return from open(), could leave a device node created and opened device_open() error handling is needlessly nested too deep, hard to read device_open() calls device_close() if a sprintf() fails? is this correct? device_close() doesn't actually close any devices, it only unlinks files Why do device_close() and device_open() use /tmp? Is there no better place? Why not use udev-populated /dev/? Easily-guessable /tmp/names lead to denial-of-service possibilities. Gatherer::~Gatherer() use-after-free, delete *i; ++i; Gatherer::getComponentTree() may leak root or ret on error FSWalk::fileScout() len is not reset to 0 on every newline extractTagValue() fails to handle the case when a tag ends with :, as in the line "power management:" archiveDB() reads an entire database into memory before compressing it, which requires the entire database to fit in RAM + swap, is this fine? Thanks -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/1417608 Title: [MIR] ppc64-diag needed in minimal for hotplug capabilities To manage notifications about this bug go to: https://bugs.launchpad.net/ubuntu/+source/libservicelog/+bug/1417608/+subscriptions -- ubuntu-bugs mailing list [email protected] https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
