On 10.08.2021 17:15, Nigel Croxon wrote:
To meet requirements of Common Criteria certification vulnerablility
assessment. Static code analysis has been run and found the following
errors:
check_return: Calling "fstat(fd, &dstb)" without checking return value.
This library function may fail and return an error code.
Changes are to add a test to the return value from fstat calls.
Hi Nigel,
You introduce three different errors, repeated many times across code.
Could you make it generic using function or macro?
diff --git a/Assemble.c b/Assemble.c
index 0df46244..cae3224b 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -649,7 +649,14 @@ static int load_devices(struct devs *devices, char *devmap,
/* prepare useful information in info structures */
struct stat stb2;
int err;
- fstat(mdfd, &stb2);
+
+ if (fstat(mdfd, &stb2) != 0) {
+ pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
+ close(mdfd);
+ free(devices);
+ free(devmap);
+ return -1;
+ }
another new case with direct error handling, could you use goto statement?
@@ -760,7 +767,17 @@ static int load_devices(struct devs *devices, char *devmap,
tst->ss->getinfo_super(tst, content, devmap + devcnt *
content->array.raid_disks);
}
- fstat(dfd, &stb);
+ if (fstat(dfd, &stb) != 0) {
+ pr_err("fstat failed for %s: %s - aborting\n",devname,
strerror(errno));
+ close(dfd);
+ close(mdfd);
+ free(devices);
+ free(devmap);
+ tst->ss->free_super(tst);
+ free(tst);
+ *stp = st;
+ return -1;
+ }
Same here, I know that it is implemented this way, but we should take care about
code quality, this is our common interest.
diff --git a/Dump.c b/Dump.c
index 736bcb60..d1a8bb86 100644
--- a/Dump.c
+++ b/Dump.c
@@ -112,7 +112,14 @@ int Dump_metadata(char *dev, char *dir, struct context *c,
}
if (c->verbose >= 0)
printf("%s saved as %s.\n", dev, fname);
- fstat(fd, &dstb);
+
+ if (fstat(fd, &dstb) != 0) {
+ pr_err("fstat failed from %s: %s\n",fname, strerror(errno));
+ close(fd);
+ close(fl);
+ free(fname);
+ return 1;
+ }
Same here.
@@ -200,7 +207,11 @@ int Restore_metadata(char *dev, char *dir, struct context
*c,
char *chosen = NULL;
unsigned int chosen_inode = 0;
- fstat(fd, &dstb);
+ if (fstat(fd, &dstb) != 0) {
+ pr_err("fstat failed for %s: %s\n",dev, strerror(errno));
+ close(fd);
+ return 1;
+ }
while (d && (de = readdir(d)) != NULL) {
if (de->d_name[0] == '.')
diff --git a/Grow.c b/Grow.c
index 7506ab46..4c3ec9c1 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1163,9 +1163,17 @@ int reshape_open_backup_file(char *backup_file,
* way this will not notice, but it is better than
* nothing.
*/
- fstat(*fdlist, &stb);
+ if (fstat(*fdlist, &stb) != 0) {
+ pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
+ close(*fdlist);
+ return 0;
+ }
dev = stb.st_dev;
- fstat(fd, &stb);
+ if (fstat(fd, &stb) != 0) {
+ pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
+ close(*fdlist);
+ return 0;
+ }
if (stb.st_rdev == dev) {
pr_err("backup file must NOT be on the array being reshaped.\n");
close(*fdlist);
Same error handling duplicated.
Thanks,
Mariusz
