bucoord: Fix problems found by static analysis

Several static analysis tools have identified various problems:
 - printf conversion specifiers                   (cppcheck)
 - sscanf string overflows                        (cppcheck)
 - files left open from early exits               (infer)
 - dereference of NULL pointer                    (scan-build)
 - missing checks to ensure *alloc was successful (infer)
 - possible memory leaks                          (scan-build, cppcheck)

To resolve the above problems:
 - update printf conversion specifiers
 - update sscanf format strings to include the size of the buffer
 - close files before exiting
 - ensure pointers are non-NULL prior to use
 - add checks to ensure *alloc was successful before using the memory
 - fix possible memory leaks by freeing memory

This commit is a reorganization of commits developed by Pat Riehecky,
who ran the static analysis tools and developed the fixes.

Change-Id: I864b2c6be0c7dec1e93b49231ad7876d38c20435
Reviewed-on: https://gerrit.openafs.org/14677
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
This commit is contained in:
Cheyenne Wills 2022-12-12 16:57:39 -07:00 committed by Michael Meffie
parent f5eb9f7452
commit 3d2e66c179
9 changed files with 177 additions and 47 deletions

View File

@ -788,7 +788,7 @@ printIfStatus(struct tciStatusS *statusPtr)
{
printf("Task %d: %s: ", statusPtr->taskId, statusPtr->taskName);
if (statusPtr->nKBytes)
printf("%ld Kbytes transferred", (long unsigned int) statusPtr->nKBytes);
printf("%lu Kbytes transferred", (long unsigned int) statusPtr->nKBytes);
if (strlen(statusPtr->volumeName) != 0) {
if (statusPtr->nKBytes)
printf(", ");
@ -1232,8 +1232,10 @@ bc_VolRestoreCmd(struct cmd_syndesc *as, void *arock)
for (ti = as->parms[5].items, i = 0; ti; ti = ti->next, i++) {
ports[i] = getPortOffset(ti->data);
if (ports[i] < 0)
if (ports[i] < 0) {
free(ports);
return (BC_BADARG);
}
}
}
@ -1362,8 +1364,10 @@ bc_DiskRestoreCmd(struct cmd_syndesc *as, void *arock)
for (ti = as->parms[2].items, i = 0; ti; ti = ti->next, i++) {
ports[i] = getPortOffset(ti->data);
if (ports[i] < 0)
if (ports[i] < 0) {
free(ports);
return (BC_BADARG);
}
}
}
@ -1378,6 +1382,7 @@ bc_DiskRestoreCmd(struct cmd_syndesc *as, void *arock)
cstruct);
if (code) {
afs_com_err(whoami, code, "; Failed to evaluate volume set");
free(ports);
return (-1);
}
@ -1497,7 +1502,7 @@ bc_VolsetRestoreCmd(struct cmd_syndesc *as, void *arock)
while (fgets(line, 255, fd)) {
count =
sscanf(line, "%s %s %s %s", server, partition, volume, rest);
sscanf(line, "%49s %49s %49s %255s", server, partition, volume, rest);
if (count <= 0)
continue;
@ -1522,11 +1527,20 @@ bc_VolsetRestoreCmd(struct cmd_syndesc *as, void *arock)
/* Allocate a volumeDump structure and link it in */
tvol = calloc(1, sizeof(struct bc_volumeDump));
if (tvol == NULL) {
fclose(fd);
afs_com_err(whoami, BC_NOMEM, NULL);
code = BC_NOMEM;
goto error;
}
tvol->name = malloc(VOLSER_MAXVOLNAME + 1);
if (!tvol->name) {
fclose(fd);
free(tvol);
afs_com_err(whoami, BC_NOMEM, NULL);
return BC_NOMEM;
code = BC_NOMEM;
goto error;
}
strncpy(tvol->name, volume, VOLSER_OLDMAXVOLNAME);
memcpy(&tvol->server, &destServer, sizeof(destServer));
@ -1552,13 +1566,16 @@ bc_VolsetRestoreCmd(struct cmd_syndesc *as, void *arock)
ports = malloc(portCount * sizeof(afs_int32));
if (!ports) {
afs_com_err(whoami, BC_NOMEM, NULL);
return BC_NOMEM;
code = BC_NOMEM;
goto error;
}
for (ti = as->parms[2].items, i = 0; ti; ti = ti->next, i++) {
ports[i] = getPortOffset(ti->data);
if (ports[i] < 0)
return (BC_BADARG);
if (ports[i] < 0) {
code = BC_BADARG;
goto error;
}
}
}
@ -1579,6 +1596,18 @@ bc_VolsetRestoreCmd(struct cmd_syndesc *as, void *arock)
afs_com_err(whoami, code, "; Failed to queue restore");
return code;
error:
while (volsToRestore != NULL) {
struct bc_volumeDump *tvol;
free(volsToRestore->name);
tvol = volsToRestore;
volsToRestore = volsToRestore->next;
free(tvol);
}
free(ports);
return code;
}
/*-----------------------------------------------------------------------------
@ -1636,17 +1665,17 @@ bc_DumpCmd(struct cmd_syndesc *as, void *arock)
code = bc_UpdateDumpSchedule();
if (code) {
afs_com_err(whoami, code, "; Can't retrieve dump schedule");
return (code);
goto exit;
}
code = bc_UpdateVolumeSet();
if (code) {
afs_com_err(whoami, code, "; Can't retrieve volume sets");
return (code);
goto exit;
}
code = bc_UpdateHosts();
if (code) {
afs_com_err(whoami, code, "; Can't retrieve tape hosts");
return (code);
goto exit;
}
/*
@ -1659,14 +1688,16 @@ bc_DumpCmd(struct cmd_syndesc *as, void *arock)
if (as->parms[0].items || as->parms[1].items || as->parms[2].items
|| as->parms[4].items) {
afs_com_err(whoami, 0, "Invalid option specified with -file option");
return -1;
code = -1;
goto exit;
}
} else {
loadfile = 0;
if (!as->parms[0].items || !as->parms[1].items) {
afs_com_err(whoami, 0,
"Must specify volume set name and dump level name");
return -1;
code = -1;
goto exit;
}
}
@ -1677,8 +1708,10 @@ bc_DumpCmd(struct cmd_syndesc *as, void *arock)
doAt = 1;
timeString = concatParams(as->parms[3].items);
if (!timeString)
return (-1);
if (timeString == NULL) {
code = -1;
goto exit;
}
/*
* Now parse this string for the time to start.
@ -1688,7 +1721,8 @@ bc_DumpCmd(struct cmd_syndesc *as, void *arock)
if (code) {
afs_com_err(whoami, 0, "Can't parse dump start date and time");
afs_com_err(whoami, 0, "%s", ktime_GetDateUsage());
return (1);
code = 1;
goto exit;
}
} else
doAt = 0;
@ -1705,14 +1739,17 @@ bc_DumpCmd(struct cmd_syndesc *as, void *arock)
/* get the port number, if one was specified */
if (as->parms[2].items) {
portp = malloc(sizeof(afs_int32));
if (!portp) {
if (portp == NULL) {
afs_com_err(whoami, BC_NOMEM, NULL);
return BC_NOMEM;
code = BC_NOMEM;
goto exit;
}
*portp = getPortOffset(as->parms[2].items->data);
if (*portp < 0)
return (BC_BADARG);
if (*portp < 0) {
code = BC_BADARG;
goto exit;
}
}
doAppend = (as->parms[4].items ? 1 : 0); /* -append */
@ -1724,14 +1761,16 @@ bc_DumpCmd(struct cmd_syndesc *as, void *arock)
if (!tvs) {
afs_com_err(whoami, 0,
"Can't find volume set '%s' in backup database", vsName);
return (-1);
code = -1;
goto exit;
}
baseds = bc_FindDumpSchedule(bc_globalConfig, dumpPath);
if (!baseds) {
afs_com_err(whoami, 0,
"Can't find dump schedule '%s' in backup database",
dumpPath);
return (-1);
code = -1;
goto exit;
}
}
@ -1788,7 +1827,8 @@ bc_DumpCmd(struct cmd_syndesc *as, void *arock)
statusPtr->cmdLine = malloc(length);
if (!statusPtr->cmdLine) {
afs_com_err(whoami, BC_NOMEM, NULL);
return BC_NOMEM;
code = BC_NOMEM;
goto exit;
}
/* Now reconstruct the dump command */
@ -1828,7 +1868,8 @@ bc_DumpCmd(struct cmd_syndesc *as, void *arock)
unlock_Status();
}
return (0);
code = 0;
goto exit;
}
/*
@ -1840,9 +1881,11 @@ bc_DumpCmd(struct cmd_syndesc *as, void *arock)
loadFile = strdup(as->parms[6].items->data);
if (!loadFile) {
afs_com_err(whoami, BC_NOMEM, NULL);
return BC_NOMEM;
code = BC_NOMEM;
goto exit;
}
return 0;
code = 0;
goto exit;
}
/*
@ -1914,11 +1957,13 @@ bc_DumpCmd(struct cmd_syndesc *as, void *arock)
code = bc_EvalVolumeSet(bc_globalConfig, tvs, &volsToDump, cstruct);
if (code) {
afs_com_err(whoami, code, "; Failed to evaluate volume set");
return (-1);
code = -1;
goto exit;
}
if (!volsToDump) {
printf("No volumes to dump\n");
return (0);
code = 0;
goto exit;
}
/* Determine what the clone time of the volume was when it was
@ -1958,8 +2003,10 @@ bc_DumpCmd(struct cmd_syndesc *as, void *arock)
for (tve = volsToDump; tve; tve = tve->next) {
printf("\t%s (%u)\n", tve->name, tve->vid);
}
if (dontExecute)
return (0);
if (dontExecute) {
code = 0;
goto exit;
}
code = bc_StartDmpRst(bc_globalConfig, dumpPath, vsName, volsToDump,
/*destServer */ NULL, /*destPartition */ 0,
@ -1970,6 +2017,11 @@ bc_DumpCmd(struct cmd_syndesc *as, void *arock)
if (code)
afs_com_err(whoami, code, "; Failed to queue dump");
/* portp is freed by bc_StartDmpRst */
portp = NULL;
exit:
free(portp);
return (code);
} /*bc_DumpCmd */

View File

@ -75,7 +75,13 @@ HostAdd(struct bc_hostEntry **alist, char *aname, afs_int32 aport)
/* tlast now points to the next pointer (or head pointer) we should overwrite */
tentry = calloc(1, sizeof(struct bc_hostEntry));
if (tentry == NULL)
return ENOMEM;
tentry->name = strdup(aname);
if (tentry->name == NULL) {
free(tentry);
return ENOMEM;
}
*tlast = tentry;
tentry->next = (struct bc_hostEntry *)0;
tentry->addr.sin_family = AF_INET;

View File

@ -200,8 +200,8 @@ tailCompPtr(char *pathNamePtr)
* afile - ptr to file, for reading.
* various - ptrs for return values
* exit:
* aname - string of form volume_set.dump_level
* dumpName - pathname of dump schedule node
* aname - string of form volume_set.dump_level (char [256])
* dumpName - pathname of dump schedule node (char [1024])
* aparent - id of parent
* aincTime
* acreateTime - time at which dump was created
@ -219,7 +219,7 @@ ScanDumpHdr(FILE *afile, char *aname, char *dumpName, afs_int32 *aparent, afs_in
if (!tp)
return -1;
code =
sscanf(tbuffer, "%d %d %s %s %ld %ld %ld %ld", &dbmagic, &dbversion,
sscanf(tbuffer, "%d %d %255s %1023s %ld %ld %ld %ld", &dbmagic, &dbversion,
aname, dumpName, (long int *) aparent, (long int *) aincTime,
(long int *) acreateTime, (long int *) alevel);
if (code != 8)
@ -255,7 +255,7 @@ afs_int32 ScanTapeVolume(FILE *afile, char *avolName, afs_int32 *avolID, char *a
return 1; /* eof */
}
code =
sscanf(tbuffer, "%s %ld %s %ld %ld %ld %ld", avolName,
sscanf(tbuffer, "%255s %ld %255s %ld %ld %ld %ld", avolName,
(long int *) avolID, atapeName, (long int *)apos,
(long int *) aseq, (long int *) alastp,
(long int *) cloneTime);

View File

@ -23,6 +23,7 @@
#include <afs/cmd.h>
#include <afs/com_err.h>
#include <afs/bubasics.h>
#include <afs/opr_assert.h>
#include "bc.h"
#include "bucoord_internal.h"
@ -191,8 +192,14 @@ bc_CreateVolumeSet(struct bc_config *aconfig, char *avolName,
/* move to end of the list */
nset = calloc(1, sizeof(struct bc_volumeSet));
if (nset == NULL)
return ENOMEM;
nset->flags = aflags;
nset->name = strdup(avolName);
if (nset->name == NULL) {
free(nset);
return ENOMEM;
}
if (aflags & VSFLAG_TEMPORARY) {
/* Add to beginning of list */
nset->next = aconfig->vset;
@ -303,20 +310,31 @@ bc_AddVolumeItem(struct bc_config *aconfig, char *avolName, char *ahost,
/* move to end of the list */
for (tentry = *tlast; tentry; tlast = &tentry->next, tentry = *tlast);
tentry = calloc(1, sizeof(struct bc_volumeEntry));
if (tentry == NULL)
return ENOMEM;
tentry->serverName = strdup(ahost);
tentry->partname = strdup(apart);
tentry->name = strdup(avol);
if (tentry->serverName == NULL || tentry->partname == NULL ||
tentry->name == NULL) {
code = ENOMEM;
goto error;
}
code = bc_ParseHost(tentry->serverName, &tentry->server);
if (code)
return (code);
goto error;
code = bc_GetPartitionID(tentry->partname, &tentry->partition);
if (code)
return (code);
goto error;
*tlast = tentry; /* thread on the list */
return 0;
error:
FreeVolumeEntry(tentry);
return code;
}
struct bc_volumeSet *
@ -365,6 +383,8 @@ bc_CreateDumpSchedule(struct bc_config *aconfig, char *adumpName,
return -2; /* name specification error */
tdump = calloc(1, sizeof(struct bc_dumpSchedule));
if (tdump == NULL)
return ENOMEM;
/* prepend this node to the dump schedule list */
tdump->next = aconfig->dsched;
@ -372,6 +392,10 @@ bc_CreateDumpSchedule(struct bc_config *aconfig, char *adumpName,
/* save the name of this dump node */
tdump->name = strdup(adumpName);
if (tdump->name == NULL) {
free(tdump);
return ENOMEM;
}
/* expiration information */
tdump->expDate = expDate;
@ -533,6 +557,8 @@ FindDump(struct bc_config *aconfig, char *nodeString,
*parentptr = 0;
*nodeptr = 0;
opr_Assert(nodeString != NULL);
/* ensure first char is correct separator */
if ((nodeString[0] != '/')
|| (strlen(&nodeString[0]) <= 1)

View File

@ -423,6 +423,10 @@ bc_ScanDumps(struct bc_config *config, afs_int32 dbAddFlag, afs_int32 port)
/* create status monitor block */
statusPtr = createStatusNode();
if (statusPtr == NULL) {
afs_com_err(whoami, ENOMEM, "; Failed to start scantape");
return ENOMEM;
}
lock_Status();
statusPtr->taskId = taskId;
statusPtr->port = port;

View File

@ -416,7 +416,7 @@ bc_ParseDumpSchedule(void)
if (tp == 0)
break; /* hit eof? */
code =
sscanf(tbuffer, "%s %s %d %d", dsname, period, &expDate,
sscanf(tbuffer, "%255s %63s %d %d", dsname, period, &expDate,
&expType);
if (code != 4) {
afs_com_err(whoami, 0,
@ -425,9 +425,15 @@ bc_ParseDumpSchedule(void)
return (BC_INTERNALERROR);
}
tds = calloc(1, sizeof(struct bc_dumpSchedule));
if (tds == NULL)
return ENOMEM;
tds->next = (struct bc_dumpSchedule *)0;
tds->name = strdup(dsname);
if (tds->name == NULL) {
free(tds);
return ENOMEM;
}
tds->expDate = expDate;
tds->expType = expType;

View File

@ -148,7 +148,7 @@ bc_Restorer(afs_int32 aindex)
char vname[BU_MAXNAMELEN];
struct budb_dumpEntry *dumpDescr, dumpDescr1, dumpDescr2;
struct budb_volumeEntry *volumeEntries;
struct budb_volumeEntry *volumeEntries = NULL;
struct bc_volumeDump *tvol;
afs_int32 code = 0, tcode;
afs_int32 tapedumpid, parent;
@ -186,6 +186,10 @@ bc_Restorer(afs_int32 aindex)
int foundtape, c;
dlevels = malloc(num_dlevels * sizeof(*dlevels));
if (dlevels == NULL) {
afs_com_err(whoami, BC_NOMEM, NULL);
ERROR(BC_NOMEM);
}
dumpTaskPtr = &bc_dumpTasks[aindex];
serverAll = HOSTADDR(&dumpTaskPtr->destServer);
@ -332,6 +336,10 @@ bc_Restorer(afs_int32 aindex)
num_dlevels += num_dlevels; /* double */
dlevels = malloc(num_dlevels * sizeof(*dlevels));
if (dlevels == NULL) {
afs_com_err(whoami, BC_NOMEM, NULL);
ERROR(BC_NOMEM);
}
memcpy(dlevels, tdl, (num_dlevels/2) * sizeof(*dlevels));
free(tdl);
}
@ -471,14 +479,14 @@ bc_Restorer(afs_int32 aindex)
tle = calloc(1, sizeof(struct bc_tapeList));
if (!tle) {
afs_com_err(whoami, BC_NOMEM, NULL);
return (BC_NOMEM);
ERROR(BC_NOMEM);
}
tle->tapeName = strdup(volumeEntries[ve].tape);
if (!tle->tapeName) {
free(tle);
afs_com_err(whoami, BC_NOMEM, NULL);
return (BC_NOMEM);
ERROR(BC_NOMEM);
}
tle->dumpID = dlevels[lv].DumpId;
@ -521,14 +529,14 @@ bc_Restorer(afs_int32 aindex)
ti = calloc(1, sizeof(struct bc_tapeItem));
if (!ti) {
afs_com_err(whoami, BC_NOMEM, NULL);
return (BC_NOMEM);
ERROR(BC_NOMEM);
}
ti->volumeName = strdup(volumeEntries[ve].name);
if (!ti->volumeName) {
free(ti);
afs_com_err(whoami, BC_NOMEM, NULL);
return (BC_NOMEM);
ERROR(BC_NOMEM);
}
ti->server = vi->server;
@ -683,7 +691,7 @@ bc_Restorer(afs_int32 aindex)
code = ConnectButc(dumpTaskPtr->config, port, &aconn);
if (code)
return (code);
ERROR(code);
if (tcarray[startentry].dumpLevel == 0)
printf("\nFull restore being processed on port %d\n", port);

View File

@ -248,7 +248,7 @@ bc_ParseHosts(void)
if (!tp)
break; /* end of file */
sscanf(tbuffer, "%s %u", hostName, &port);
sscanf(tbuffer, "%255s %u", hostName, &port);
th = gethostbyname(hostName);
if (th == 0) {
afs_com_err(whoami, 0,
@ -256,8 +256,11 @@ bc_ParseHosts(void)
hostName);
}
the = calloc(1, sizeof(struct bc_hostEntry));
if (the == (struct bc_hostEntry *)0)
if (the == NULL) {
bc_globalConfig->tapeHosts = tfirst;
bc_ClearHosts();
return (BC_NOMEM);
}
if (tlast) {
tlast->next = the;
tlast = the;
@ -266,6 +269,11 @@ bc_ParseHosts(void)
}
the->next = (struct bc_hostEntry *)0;
the->name = strdup(hostName);
if (the->name == NULL) {
bc_globalConfig->tapeHosts = tfirst;
bc_ClearHosts();
return (BC_NOMEM);
}
the->portOffset = port;
if (th) {
memcpy(&the->addr.sin_addr.s_addr, th->h_addr, 4);

View File

@ -472,7 +472,7 @@ bc_ParseVolumeSet(void)
* Scan a header entry.
*/
readHeader = 0;
code = sscanf(tbuffer, "%s %s", serverName, vsname);
code = sscanf(tbuffer, "%255s %255s", serverName, vsname);
if ((code != 2)
|| (strcmp(serverName, "volumeset") != 0)
) {
@ -485,7 +485,18 @@ bc_ParseVolumeSet(void)
* global configuration structure.
*/
tvs = calloc(1, sizeof(struct bc_volumeSet));
if (tvs == NULL) {
afs_com_err(whoami, 0,
"Can't malloc() a new volume set!");
return -1;
}
tvs->name = strdup(vsname);
if (tvs->name == NULL) {
free(tvs);
afs_com_err(whoami, 0,
"Can't malloc() a new volume set name!");
return -1;
}
/* append to the end */
for (ppvs = &bc_globalConfig->vset, pvs = *ppvs; pvs;
@ -497,7 +508,7 @@ bc_ParseVolumeSet(void)
/* Scan a volume name entry, which contains the server name,
* partition pattern, and volume pattern.
*/
code = sscanf(tbuffer, "%s %s %s", serverName, partName, vsname);
code = sscanf(tbuffer, "%255s %255s %255s", serverName, partName, vsname);
if (code == 1 && strcmp(serverName, "end") == 0) {
/* This was really a line delimiting the current volume set.
* Next time around, we should try to read a header.
@ -525,23 +536,32 @@ bc_ParseVolumeSet(void)
*/
tve->serverName = strdup(serverName);
if (!tve->serverName) {
free(tve);
afs_com_err(whoami, 0,
"Can't malloc() a new volume spec server name field!");
return (-1);
}
tve->partname = strdup(partName);
if (!tve->partname) {
free(tve->serverName);
free(tve);
afs_com_err(whoami, 0,
"Can't malloc() a new volume spec partition pattern field!");
return (-1);
}
code = bc_GetPartitionID(partName, &tve->partition);
if (code) {
free(tve->serverName);
free(tve->partname);
free(tve);
afs_com_err(whoami, 0, "Can't parse partition '%s'", partName);
return -1;
}
tp = strdup(vsname);
if (!tp) {
free(tve->serverName);
free(tve->partname);
free(tve);
afs_com_err(whoami, 0,
"Can't malloc() a new volume spec volume pattern field!");
return (-1);