[prev in list] [next in list] [prev in thread] [next in thread] 

List:       trousers-tech
Subject:    Re: [TrouSerS-tech] [PATCH 10/17] Removing possibility of NULL-dereferencing pointer
From:       "Fuchs, Andreas" <andreas.fuchs () sit ! fraunhofer ! de>
Date:       2014-04-11 14:06:12
Message-ID: 1397225171.15139.65.camel () pc-fuchslap2 ! sit ! fraunhofer ! de
[Download RAW message or body]

Am Freitag, den 11.04.2014, 10:43 -0300 schrieb Richard:
> Em 11-04-2014 06:49, Fuchs, Andreas escreveu:
> > Disclaimer:
> > I could not complie-test or runtime-test these patches right now. This is a pure \
> > code-only review of the patches. 
> > If toKill was actually NULL, you'd already die on line
> > 125:		previous->next = toKill->next;
> 
> Line 123 does a check on that one 'else if (previous && toKill) {'

guess I missed that one.

> 
> > If get_context() or get_previous_context() can actually return NULL, I'd add a \
> > return ERROR; right after those two functions. Otherwise, I guess coverity is \
> > just not intelligent enough. Still, a NULL-check at the beginning right after the \
> > two getters is more logical (even if it's only for coverity).
> It doesn't return an error, because it's still tasked to unlock 
> 'tcs_ctx_lock', even if both returned contexts are NULL.
> Check line 127 for that.
> 
> The person who created this code didn't do that (IMO), because it did 
> want to separate the test in line 137, so it could be enclosed by the 
> ifdef in line 135. Code legibility suffered for that.

Actually, by doing the math, line 127 is almost the case of (toKill == previous == \
NULL)... Hard to see... :-/

So when reaching line 137, toKill could only be NULL, if previous == NULL and \
tcs_context_table->handle == handle in which case it would we weird that \
get_context(handle) == NULL. So, I guess no toKill == NULL in line 137, but the patch \
can be applied for coverity anyways, I guess...


By the way, can tcs_context_table == NULL ? That would be "ungood" in line 120.


> > 
> > Am Mittwoch, den 09.04.2014, 15:41 -0300 schrieb rmaciel@linux.vnet.ibm.com:
> > > From: Richard Maciel <rmaciel@linux.vnet.ibm.com>
> > > 
> > > Related to coverity CID 10304.
> > > 
> > > There was a possible code execution path, in function context_destroy
> > > that have toKill pointer var with the NULL value.
> > > 
> > > Signed-off-by: Richard Maciel <rmaciel@linux.vnet.ibm.com>
> > > ---
> > > src/tcs/tcs_context.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/src/tcs/tcs_context.c b/src/tcs/tcs_context.c
> > > index 905567b..2072bdc 100644
> > > --- a/src/tcs/tcs_context.c
> > > +++ b/src/tcs/tcs_context.c
> > > @@ -134,7 +134,7 @@ destroy_context(TCS_CONTEXT_HANDLE handle)
> > > 
> > > #ifdef TSS_BUILD_TRANSP
> > > 	/* Free existing transport session if necessary */
> > > -	if (toKill->transHandle)
> > > +	if (toKill != NULL && toKill->transHandle)
> > > 		TCSP_FlushSpecific_Common(toKill->transHandle, TPM_RT_TRANS);
> > > #endif
> > > 
> 

------------------------------------------------------------------------------
Put Bad Developers to Shame
Dominate Development with Jenkins Continuous Integration
Continuously Automate Build, Test & Deployment 
Start a new project now. Try Jenkins in the cloud.
http://p.sf.net/sfu/13600_Cloudbees
_______________________________________________
TrouSerS-tech mailing list
TrouSerS-tech@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/trousers-tech


[prev in list] [next in list] [prev in thread] [next in thread]