Raise the warning level to -Wall -Wextra and resolve the warnings - #609
Raise the warning level to -Wall -Wextra and resolve the warnings#609matejk wants to merge 3 commits into
Conversation
- Check or deliberately consume warn_unused_result returns: asprintf results are verified instead of testing the (undefined-on-failure) output pointer; the lost-and-found path no longer leaks or reuses the old path on reallocation; the trace dump consumes write results while staying best-effort; /proc reads fall back to a placeholder and leave room for the terminator and arch suffix. - Replace strncpy on fixed-width SCSI and MAM fields with memcpy or the new bounded ltfs_string_copy (tape_ops.h), which always terminates; the device-list and serial-number copies were not guaranteed to be terminated before. The device-provided serial length is clamped to the field. - The MAM coherency signature copies its terminator explicitly; the reader checks all five bytes. - Initialize values read on early-exit paths (u_get_truncate_size). - Use size_t for the name-length arithmetic in fs_dentry_lookup; the unsigned conversion also surfaced and fixed a reversed loop bound in the cleanup path. - Spell the fall-through comment so the compiler recognizes it; format the XML timestamp with a real snprintf instead of the size-discarding arch_sprintf. Addresses the -Wstringop-truncation reports of LinearTapeFileSystem#280.
unused-parameter and missing-field-initializers stay disabled: callback signatures must keep parameters they do not use, and the operation/option tables intentionally rely on zero-initialized remaining members. The tree builds without compiler warnings on Linux (gcc 15, -O0 and -O2) and macOS (clang) apart from macOS SDK deprecation notices. The CMake build carries the same flags.
| compile_commands.json | ||
| .cache/ |
There was a problem hiding this comment.
This seems like a change done for the use of Cmake, should be moved to that PR since it's also not there and removed from here since it's unrelated.
| dnl must keep parameters they do not use. | ||
| dnl -Wno-missing-field-initializers: designated initializers of operation and | ||
| dnl option tables intentionally leave the remaining members zeroed. | ||
| AM_CFLAGS="-Wall -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wsign-compare -fsigned-char ${FUSE_MODULE_CFLAGS} ${UUID_MODULE_CFLAGS} ${LIBXML2_MODULE_CFLAGS} ${ICU_MODULE_CFLAGS} ${SNMP_ENABLE} ${SNMP_MODULE_CFLAGS}" |
There was a problem hiding this comment.
This change overwrite whatever AM_CFLAGS contained before that could be carried from the environment. As the code below this and many others you should first add ${AM_CFLAGS} before adding others to avoid the overwrite.
| /* The trace dump is best-effort diagnostics; a write failure must not fail | ||
| * the operation that triggered the dump. glibc marks write() with | ||
| * warn_unused_result and a (void) cast does not silence it, so consume the | ||
| * result here and deliberately ignore it. */ | ||
| static void _dump_write(int fd, const void *buf, size_t count) | ||
| { | ||
| ssize_t ignored = arch_write(fd, buf, count); | ||
| (void)ignored; | ||
| } | ||
|
|
There was a problem hiding this comment.
This kind of solution that is just an static file single-wrapper and it's not done system wide seems a little bit overengineered. It hides the intent from the call sites and can't be reused anywhere else. A more idiomatic approach should be used like (void)!arch_write(...) at the call sites instead.
| /* MAM attribute fields have fixed widths; truncation is intentional and | ||
| * parse_vol() pads and terminates afterwards. */ | ||
| static void mam_field_copy(char *dest, size_t field_size, const char *src) | ||
| { | ||
| size_t n = strlen(src); | ||
|
|
||
| if (n > field_size) | ||
| n = field_size; | ||
| memcpy(dest, src, n); | ||
| } | ||
|
|
There was a problem hiding this comment.
Same issue, single file solutions are not the best solutions, this also doesn't null terminate making the use of the function dependent on parse_vol() being called afterwards and not reusable. memcpy should be used instead in place instead of a custom wrapper. This also duplicates semantics already used in arch_strncpy.
| /* Bounded, always-terminated string copy for the fixed-width identifier | ||
| * fields below; truncation of longer sources is intentional. */ | ||
| static inline void ltfs_string_copy(char *dest, size_t dest_size, const char *src) | ||
| { | ||
| size_t n = strnlen(src, dest_size - 1); | ||
|
|
||
| memcpy(dest, src, n); | ||
| dest[n] = '\0'; | ||
| } |
There was a problem hiding this comment.
This has the same issue, just a little bit more wide use. Also it adds a third signature to the copy functions of the codebase making it harder for other contributors at knowing which one to use. This can be solved using other more idiomatic approaches like sprintf, or any other. Prefer to use those instead or find a solution that doesn't require a custom copy function.
| memcpy(work_buf+len+1, token, strlen(token)); /* "/aaa/ccc\0" */ | ||
| work_buf[len+1+strlen(token)] = '\0'; |
There was a problem hiding this comment.
This adds a new problem because now the bound put by work_buf_len is not there, making it possible for a buffer overwrite risk, consider other solution for the warning.
| memcpy(work_buf+len+1, token, strlen(token)); /* "/aaa/ccc/target.txt\0" */ | ||
| work_buf[len+1+strlen(token)] = '\0'; |
There was a problem hiding this comment.
Ditto about the problem with dropping work_buf_len.
2 participants
The build used the default warning level, which hides real defects. This PR adds
-Wall -WextratoAM_CFLAGSand resolves every warning the tree then produces — string truncation in MAM attribute handling, sign and width mismatches, unused parameters and variables, missing field initializers — without behavior changes. Genuine bugs discovered by the higher warning level are filed in the separate bugfix PRs rather than here, so this PR stays mechanical and reviewable.Rebased onto the v2.4.8.4 release: that release independently fixed some of the same unchecked-
asprintfwarnings (inxattr.c,ltfs_internal.c,filedebug_tc.c), so those hunks are now dropped; this PR retains the-Wall -Wextraenablement and the remaining fixes (tape.c,ltfstrace.c,xml_writer.c,xml_writer_libltfs.c,itdtimg_tc.c,sg_tape.c,sg_scsi_tape.c,arch_info.c,fs.c,ltfs_fsops.c,tape_ops.h).Verified warning-clean at
-Wall -Wextra -O2with gcc on Ubuntu and clang on macOS (arm64);make checkstays green.Fixes #280.