Potential free uninitialized pointer in kern_ktrace.c

[Mark Latimer]

Hello,

reading through the compiler warnings I believe there is a potential issue
in /usr/src/sys/kern/kern_ktrace.c At first glance it appears to free
an uninitialized pointer memp.

Regards

int
ktruser(struct proc *p, const char *id, const void *addr, size_t len)
{
struct ktr_header kth;
struct ktr_user ktp;
int error;
//uninitalized
void *memp;
#define STK_PARAMS  128
long long stkbuf[STK_PARAMS / sizeof(long long)];

if (!KTRPOINT(p, KTR_USER))
return (0);
if (len  KTR_USER_MAXLEN)
return (EINVAL);

atomic_setbits_int(p-p_flag, P_INKTR);
ktrinitheader(kth, p, KTR_USER);
memset(ktp.ktr_id, 0, KTR_USER_MAXIDLEN);
error = copyinstr(id, ktp.ktr_id, KTR_USER_MAXIDLEN, NULL);
//if error then skip setting memp
if (error)
goto out;

if (len  sizeof(stkbuf))
memp = malloc(len, M_TEMP, M_WAITOK);
else
memp = stkbuf;
error = copyin(addr, memp, len);
if (error)
goto out;

ktrwrite2(p, kth, ktp, sizeof(ktp), memp, len);
out:
// frees the uninitialized pointer
if (memp != stkbuf)
free(memp, M_TEMP, len);

Re: Potential free uninitialized pointer in kern_ktrace.c

[Doug Hogan]

On Sat, Aug 01, 2015 at 07:31:58PM +0100, Mark Latimer wrote:
 reading through the compiler warnings I believe there is a potential issue
 in /usr/src/sys/kern/kern_ktrace.c At first glance it appears to free
 an uninitialized pointer memp.

I agree.


Index: sys/kern/kern_ktrace.c
===
RCS file: /cvs/src/sys/kern/kern_ktrace.c,v
retrieving revision 1.74
diff -u -p -r1.74 kern_ktrace.c
--- sys/kern/kern_ktrace.c  19 Jul 2015 04:45:25 -  1.74
+++ sys/kern/kern_ktrace.c  1 Aug 2015 18:55:44 -
@@ -348,7 +348,7 @@ ktruser(struct proc *p, const char *id, 
struct ktr_header kth;
struct ktr_user ktp;
int error;
-   void *memp;
+   void *memp = NULL;
 #defineSTK_PARAMS  128
long long stkbuf[STK_PARAMS / sizeof(long long)];
 

Re: Potential free uninitialized pointer in kern_ktrace.c

[Todd C. Miller]

On Sat, 01 Aug 2015 12:12:01 -0700, Philip Guenther wrote:

 Since my error was moving code across a goto, I'm inclined to kill the
 goto completely, like this:

That is easier to follow to boot. OK millert@

 - todd

Re: Potential free uninitialized pointer in kern_ktrace.c

[Philip Guenther]

On Sat, Aug 1, 2015 at 12:05 PM, Doug Hogan d...@acyclic.org wrote:
 On Sat, Aug 01, 2015 at 07:31:58PM +0100, Mark Latimer wrote:
 reading through the compiler warnings I believe there is a potential issue
 in /usr/src/sys/kern/kern_ktrace.c At first glance it appears to free
 an uninitialized pointer memp.

 I agree.

Since my error was moving code across a goto, I'm inclined to kill the
goto completely, like this:


--- kern_ktrace.c   19 Jul 2015 04:45:25 -  1.74
+++ kern_ktrace.c   1 Aug 2015 18:51:10 -
@@ -361,21 +361,17 @@ ktruser(struct proc *p, const char *id,
ktrinitheader(kth, p, KTR_USER);
memset(ktp.ktr_id, 0, KTR_USER_MAXIDLEN);
error = copyinstr(id, ktp.ktr_id, KTR_USER_MAXIDLEN, NULL);
-   if (error)
-   goto out;
-
-   if (len  sizeof(stkbuf))
-   memp = malloc(len, M_TEMP, M_WAITOK);
-   else
-   memp = stkbuf;
-   error = copyin(addr, memp, len);
-   if (error)
-   goto out;
-
-   ktrwrite2(p, kth, ktp, sizeof(ktp), memp, len);
-out:
-   if (memp != stkbuf)
-   free(memp, M_TEMP, len);
+   if (error == 0) {
+   if (len  sizeof(stkbuf))
+   memp = malloc(len, M_TEMP, M_WAITOK);
+   else
+   memp = stkbuf;
+   error = copyin(addr, memp, len);
+   if (error == 0)
+   ktrwrite2(p, kth, ktp, sizeof(ktp), memp, len);
+   if (memp != stkbuf)
+   free(memp, M_TEMP, len);
+   }
atomic_clearbits_int(p-p_flag, P_INKTR);
return (error);
 }