https://bugzilla.redhat.com/show_bug.cgi?id=1835452
Spencer Lingard spencer@mellanox.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(spencer@mellanox. | |com) |
--- Comment #6 from Spencer Lingard spencer@mellanox.com ---
Spec URL: https://download.copr.fedorainfracloud.org/results/slingard/mlxbf-bootctl/fe... SRPM URL: https://download.copr.fedorainfracloud.org/results/slingard/mlxbf-bootctl/fe... Scratch Koji URL: https://koji.fedoraproject.org/koji/taskinfo?taskID=45835273
New version for review.
(In reply to Honggang LI from comment #2)
1 Name: mlxbf-bootctl 2 Version: 1.1 3 %{!?_release: %define _release 4} 4 Release: %{_release}%{?dist}Please delete line 3, and replace "%{_release}" with 4 for line 4.
Done.
5 Summary: Mellanox BlueField boot partition management utility 6 7 License: BSD 8 Url: https://github.com/Mellanox/mlxbf-bootctl 9 Source: mlxbf-bootctl-1.1.tar.gz 10 11 ExclusiveArch: aarch64Need a comment for "ExclusiveArch", see https://fedoraproject.org/wiki/Packaging:ReviewGuidelines
Done.
12 13 BuildRequires: binutils 14 BuildRequires: gccline 13 should be deleted, as gcc requires binutils.
$ rpm -qR gcc | grep binutils binutils >= 2.31
Done.
15 16 %description 17 Access to all the boot partition management is via a program shipped 18 with the BlueField software called "mlxbf-bootctl".I have no idea what is main function or feature of this package after read this '%description' section. Please improve it.
Done.
19 20 %prep 21 %setup -q -n mlxbf-bootctl-1.1"%setup -q" should be enough, in case
- top directory name was in format "%{name}-%{version}/"
- tarball name was in format "%{name}-%{version}.XXX"
This spec file is generated by rpkg (https://github.com/Mellanox/mlxbf-bootctl/blob/master/mlxbf-bootctl.spec.rpk...). As far as I can tell, I can't change the way rpkg generates the %setup macro.
22 23 %build 24 %make_build 25 26 %install 27 %make_install 28 %{__install} -d %{buildroot}%{_mandir}/man8 29 %{__install} -m 0644 mlxbf-bootctl.8 %{buildroot}%{_mandir}/man8 30 31 %files 32 %defattr(-, root, root)line 32 is unnecessary, please remove it.
Done.
33 /sbin/*should install programs in %{_sbindir}, and use %{_sbindir}/XXX, XXX is the program name.
Done.
34 %{_mandir}/man8/mlxbf-bootctl.8.gz 35 36 %license LICENSE 37 %doc mlxbf-bootctl.txt 38 39 %changelog 40 * Wed Jun 10 2020 Spencer Lingard <spencer@mellanox.com> 1.1-4 41 (none) 42 43 * Tue May 12 2020 Spencer Lingard <spencer@mellanox.com> 1.1-3 44 (none) line 41 and 44 are unnecessary, should be deleted.
Changed them to be more detailed.
(In reply to Honggang LI from comment #3)
Task URL: https://cov01.lab.eng.brq.redhat.com/covscanhub/task/175416/ Comment: None
All defects
CHECKED_RETURN 1 CLANG_WARNING 1 CPPCHECK_WARNING 1
========================================================================== mlxbf-bootctl-1.1-4.fc31 List of Defects
Error: CPPCHECK_WARNING (CWE-664): [#def1] mlxbf-bootctl-1.1/mlxbf-bootctl.c:60: error[va_end_missing]: va_list 'ap' was opened but not closed by va_end(). # 58| putc('\n', stderr); # 59| exit(1); # 60|-> } # 61| # 62| #ifndef OUTPUT_ONLY
Fixed.
Error: CLANG_WARNING: [#def2] mlxbf-bootctl-1.1/mlxbf-bootctl.c:545:5: warning: Null pointer passed to 1st parameter expecting 'nonnull' # memset(buf + seg_size, 0, pad_size); # ^
This is a false positive. The tool assumes that (buf == NULL) at line 524, but if buf == NULL, the program will die with an error message rather than continuing. See the note below.
mlxbf-bootctl-1.1/mlxbf-bootctl.c:644:10: note: Assuming the condition is true # while ((opt = getopt_long(argc, argv, short_options, long_options, NULL)) # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:644:3: note: Loop condition is true. Entering loop body # while ((opt = getopt_long(argc, argv, short_options, long_options, NULL)) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:647:5: note: Control jumps to 'case 98:' at line 663 # switch (opt) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:665:7: note: Execution continues on line 644 # break; # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:644:10: note: Assuming the condition is false # while ((opt = getopt_long(argc, argv, short_options, long_options, NULL)) # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:644:3: note: Loop condition is false. Execution continues on line 694 # while ((opt = getopt_long(argc, argv, short_options, long_options, NULL)) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:694:7: note: Assuming 'bootstream' is non-null # if (!bootstream && !swap && watchdog_swap == NULL && !watchdog_disable) # ^~~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:694:19: note: Left side of '&&' is false # if (!bootstream && !swap && watchdog_swap == NULL && !watchdog_disable) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:700:7: note: 'bootstream' is non-null # if (bootstream) # ^~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:700:3: note: Taking true branch # if (bootstream) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:702:9: note: 'input_file' is null # if (input_file) # ^~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:702:5: note: Taking false branch # if (input_file) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:707:14: note: 'output_file' is null # else if (output_file) # ^~~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:707:10: note: Taking false branch # else if (output_file) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:718:11: note: Assuming 'boot_part_size' is
= field 'st_size'
# if (st.st_size > boot_part_size) # ^~~~~~~~~~~~~~~~~~~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:718:7: note: Taking false branch # if (st.st_size > boot_part_size) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:725:11: note: Assuming the condition is false # if (asprintf(&bootfile, "%sboot%d", mmc_path, boot_part ^ which_boot) <= 0) # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:725:7: note: Taking false branch # if (asprintf(&bootfile, "%sboot%d", mmc_path, boot_part ^ which_boot) <= 0) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:728:7: note: Calling 'write_bootstream' # write_bootstream(bootstream, bootfile, O_SYNC); # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:475:3: note: Taking true branch # if (strncmp(bootfile, "/dev/", 5) == 0) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:477:9: note: Assuming the condition is false # if (asprintf(&sysname, "/sys/block/%s/force_ro", &bootfile[5]) <= 0) # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:477:5: note: Taking false branch # if (asprintf(&sysname, "/sys/block/%s/force_ro", &bootfile[5]) <= 0) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:481:9: note: Assuming 'sysfd' is < 0 # if (sysfd >= 0) # ^~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:481:5: note: Taking false branch # if (sysfd >= 0) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:503:11: note: Assuming the condition is false # if (errno != ENOENT) # ^~~~~~~~~~~~~~~ /usr/include/errno.h:38:16: note: expanded from macro 'errno' ## define errno (*__errno_location ()) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:503:7: note: Taking false branch # if (errno != ENOENT) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:513:7: note: Assuming 'ifd' is < 0 # if (ifd < 0) # ^~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:513:3: note: Taking true branch # if (ifd < 0) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:516:7: note: Assuming 'ofd' is >= 0 # if (ofd < 0) # ^~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:516:3: note: Taking false branch # if (ofd < 0) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:519:7: note: Assuming the condition is false # if (fstat(ifd, &st) < 0) # ^~~~~~~~~~~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:519:3: note: Taking false branch # if (fstat(ifd, &st) < 0) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:523:3: note: 'buf' initialized here # char *buf = malloc(MAX_SEG_LEN); # ^~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:524:7: note: Assuming 'buf' is equal to NULL # if (buf == NULL) # ^~~~~~~~~~~
Note: if buf == NULL here, the true branch will stop the program, and a NULL pointer will not be passed to memset.
mlxbf-bootctl-1.1/mlxbf-bootctl.c:524:3: note: Taking true branch # if (buf == NULL) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:532:10: note: Assuming 'bytes_left' is > 0 # while (bytes_left > 0) # ^~~~~~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:532:3: note: Loop condition is true. Entering loop body # while (bytes_left > 0) # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:534:24: note: Assuming the condition is false # size_t seg_size = (bytes_left <= MAX_SEG_LEN) ? bytes_left : MAX_SEG_LEN; # ^~~~~~~~~~~~~~~~~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:534:23: note: '?' condition is false # size_t seg_size = (bytes_left <= MAX_SEG_LEN) ? bytes_left : MAX_SEG_LEN; # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:538:23: note: '?' condition is false # size_t pad_size = seg_size % 8 ? (8 - seg_size % 8) : 0; # ^ mlxbf-bootctl-1.1/mlxbf-bootctl.c:539:41: note: 'bytes_left' is not equal to 0 # uint64_t segheader = gen_seg_header(bytes_left == 0, 1, BOOT_FIFO_ADDR, # ^~~~~~~~~~ mlxbf-bootctl-1.1/mlxbf-bootctl.c:545:5: note: Null pointer passed to 1st parameter expecting 'nonnull' # memset(buf + seg_size, 0, pad_size); # ^ ~~~~~~~~~~~~~~ # 543| // Copy the segment plus any padding. # 544| read_or_die(bootstream, ifd, buf, seg_size); # 545|-> memset(buf + seg_size, 0, pad_size); # 546| write_or_die(bootfile, ofd, buf, seg_size + pad_size); # 547| }
Error: CHECKED_RETURN (CWE-252): [#def3] mlxbf-bootctl-1.1/mlxbf-bootctl.c:717: check_return: Calling "stat(bootstream, &st)" without checking return value. This library function may fail and return an error code. [Note: The source code implementation of the function has been overridden by a builtin model.] # 715| uint64_t boot_part_size = get_boot_partition_size(); # 716| struct stat st; # 717|-> stat(bootstream, &st); # 718| if (st.st_size > boot_part_size) # 719| die("Size of bootstream exceeds boot partition size");
Fixed.