Pavel Raiskup found the following defect in setcifsacl with Coverity: "segfault may occur also in cifs-utils-4.8.1/setcifsacl.c|644| because of casesptr dereferencing. When you look e.g. at the line 605, in this time any part of 'caseptr' may be yet uninitialized and program is going through 'goto' to freeing -> and there you are freeing the 'caseptr[i]' address."
The analysis there seems a little off, but is basically correct. The freeing loop counts down from the current value of i to free the secondary allocations here. There is one situation though where this could go badly. If the strtok parsing near the beginning of the loop fails, then we could end up trying to free an uninitialized pointer. Fix this by changing the cacesptr allocation to use calloc(), and stop trying to be clever with the freeing loop. Just have it walk the entire array and attempt to free each slot. Reported-by: Pavel Raiskup <[email protected]> Signed-off-by: Jeff Layton <[email protected]> --- setcifsacl.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/setcifsacl.c b/setcifsacl.c index 9dfdad7..38d003e 100644 --- a/setcifsacl.c +++ b/setcifsacl.c @@ -586,8 +586,7 @@ build_cmdline_aces(char **arrptr, int numcaces) char *acesid, *acetype, *aceflag, *acemask; struct cifs_ace **cacesptr; - cacesptr = (struct cifs_ace **)malloc(numcaces * - sizeof(struct cifs_aces *)); + cacesptr = calloc(numcaces, sizeof(struct cifs_aces *)); if (!cacesptr) { printf("%s: Error %d allocating ACE array", __func__, errno); return NULL; @@ -639,7 +638,7 @@ build_cmdline_aces(char **arrptr, int numcaces) return cacesptr; build_cmdline_aces_ret: - for (; i >= 0; --i) + for (i = 0; i < numcaces; ++i) free(cacesptr[i]); free(cacesptr); return NULL; -- 1.7.11.7 -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
