[389-devel] Re: Please review, svrcore fix coverity issues 6 through 10

On 04/11/2016 10:30 PM, William Brown wrote:

https://pagure.io/svrcore/pull-request/11    Patch bundle to fix coverity issues discovered during build processes.     

--  389-devel mailing list  389-devel@%(host_name)s  http://lists.fedoraproject.org/admin/lists/389-devel@lists.fedoraproject.org

Maybe, it'd be nice to give us this link, too.  (First, I was not sure what to review... :)

https://pagure.io/svrcore/pull-request/11.patch

The patch page does not provide the nice coloring as on 389 trac.  Could there be any way to configure it (well, that'd be a question to myself, though)?  Until we figure it out, could you please attach the patch to the original trac ticket 389ds #48450?

I have some questions on the patches...

1. Deadcode section has reported 2 defects in getPin (systemd-ask-pass.c).

# 210| err = SVRCORE_IOOperationError;

# 211| goto out;

# 212|-> free(token);

# 213| token = NULL;

# 214| }

The variable "token" is malloced in getPin and it's freed in the first 3 errors.  I see more error cases which do not free token.  Are they okay?  Probably, that's what "1. Defect type: CLANG_WARNING" is pointing out?

244     if (tmp_fd == NULL) {
245         fprintf(stderr, "SVRCORE systemd:getPin() -> opening ask file FAILED\n");
246         err = SVRCORE_IOOperationError;
247         goto out;
248     }

280     if (rename(tmp_path, ask_path) != 0) {
281         fprintf(stderr, "SVRCORE systemd:getPin() -> renaming ask file FAILED %d\n", err);
282         err = SVRCORE_IOOperationError;
283         goto out;
284     }

....

Also, since this is not slapd_ch_malloc, you may want to check the return value of malloc?

170     char *token = malloc(PASS_MAX);

2. I see this change for the fix for "Use after Free".  But *out still points the memory obj and to be returned after it's freed.

@@ -164,20 +138,26 @@ SVRCORE_CreateStdSystemdPinObj(
         obj->top = top;
     } while(0);
 
+    *out = obj;
+
     if (err != SVRCORE_Success)
     {
         SVRCORE_DestroyStdSystemdPinObj(obj);
     }
 
-    *out = obj;
-
     return err;

I think you sould set *out = NULL if (err != SVRCORE_Success)?