TPM_ERROR and ScopedTssType: uses the first, creates the second.

TPM_ERROR and ScopedTssType: uses the first, creates the second.

This change fixes two issues:
1. A huge amount of duplicated code during error path handling.
2. Relying on TSS_SUCCESS to be 0 forever.

2 was fixed by using the TPM_ERROR() macro everywhere.

1 is addressed by adding wrappers around all the TSS objects that need
to be freed such that automatic variable scope will result in the
correct releasing of the objects (and in the right order).

My original implementation used one main TSS manager class which
handled the objects and memory.  However, ignoring storage class
changes, the destructor ordering in a function should be reliable
to ensure that all TSS objects and memory will be cleaned up
prior to any ScopedTssContexts being destroyed.

Thoughts?

TEST=
1. unittests pass
2. suite_Smoke passes in a VM
3. live test of x86-alex works
4. On booted x86-alex, repeated calls to cryptohome --action=status does not appear to incur any memory leakage.  I'll start a longer run.

BUG=chromium-os:13129
Change-Id: I5b0f654cc524fb4fee4f909a4d7d4c9754595c69

R=gauravsh@chromium.org,fes@chromium.org

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=a50e4c5

[gauravsh, 2011-03-29 19:16:05]

This is a nice change.

My main comment is regarding the naming and semantics of the Scoped* types. They
behave as a pointer, so maybe their name should reflect that. More importantly
though, the var names should reflect this distinction - at some places
context_handle is a TSS_HCONTEXT, at others it behaves like a TSS_HCONTEXT*.
This gets a bit confusing to read.

(And yes, I do realize that trousers itself is pretty inconsistent about this
naming, but that can be fixed. :) )

http://codereview.chromium.org/6683071/diff/10002/scoped_tss_type.h
File scoped_tss_type.h (right):

http://codereview.chromium.org/6683071/diff/10002/scoped_tss_type.h#newcode9
scoped_tss_type.h:9: //  ScopedTssContext context;
context_handle

http://codereview.chromium.org/6683071/diff/10002/scoped_tss_type.h#newcode13
scoped_tss_type.h:13: //  ScopedTssKey srk(*context);
context_handle

http://codereview.chromium.org/6683071/diff/10002/scoped_tss_type.h#newcode47
scoped_tss_type.h:47: Tspi_Context_FreeMemory(context, memory);
so in ScopedTssContextRelease, you check for (context) before calling close on
it. Any particularly reason why you don't this here.

Same comments for ScopeTssObjectRelease below (check for context and handle)

http://codereview.chromium.org/6683071/diff/10002/tpm.cc
File tpm.cc (right):

http://codereview.chromium.org/6683071/diff/10002/tpm.cc#newcode319
tpm.cc:319: ScopedTssKey local_key_handle(context_handle);
It is a bit confusing due to different semantics of the Scoped* data structures
- i.e. ScopedTssKey does not behave like a TSS_HKEY, it behaves like TSS_HKEY*.
Which is fine but you may consider renaming it make it reflect that
(ScopedTssMemoryPtr) or something.

Another point: context_handle is used in different parts of the file to mean 2
different things (TSS_HCONTEXT) and ScopesTssContext. Since the semantics of
them are different, this gets confusing (I initially thought you had made a
mistake and it should be *context_handle as in the previous function). I quickly
realized that here context_handle is an actual HCONTEXT. Use different var names
to make that distinction clearer.

http://codereview.chromium.org/6683071/diff/10002/tpm.cc#newcode322
tpm.cc:322: init_flags,
nit: indent is off

http://codereview.chromium.org/6683071/diff/10002/tpm.cc#newcode331
tpm.cc:331: TSS_TSPATTRIB_KEY_INFO,
nit: indent is off

http://codereview.chromium.org/6683071/diff/10002/tpm.cc#newcode393
tpm.cc:393: *local_key_handle,
nit: could move this to the line above.

http://codereview.chromium.org/6683071/diff/10002/tpm.cc#newcode403
tpm.cc:403: *local_key_handle,
nit: could move this to the line above.

http://codereview.chromium.org/6683071/diff/10002/tpm.cc#newcode905
tpm.cc:905: static_cast<BYTE *>(
ahh, the joys of type casting.

[Will Drewry, 2011-03-29 19:42:21]

Fixed everything except the ScopedTssType naming issue.  Any thoughts on what
would be more clear would be appreciated. The current use is a bit ungainly.  (I
may see what get() and pointer() might look like.)  I guess the other option of
treating it as a pre-existing pointer works, if that would make it make more
sense as is.

thanks!
will

http://codereview.chromium.org/6683071/diff/10002/scoped_tss_type.h
File scoped_tss_type.h (right):

http://codereview.chromium.org/6683071/diff/10002/scoped_tss_type.h#newcode9
scoped_tss_type.h:9: //  ScopedTssContext context;
On 2011/03/29 19:16:06, gauravsh wrote:
> context_handle

Ugh. I hate that variable name, but okay. :)

http://codereview.chromium.org/6683071/diff/10002/scoped_tss_type.h#newcode13
scoped_tss_type.h:13: //  ScopedTssKey srk(*context);
On 2011/03/29 19:16:06, gauravsh wrote:
> context_handle

Done.

http://codereview.chromium.org/6683071/diff/10002/scoped_tss_type.h#newcode47
scoped_tss_type.h:47: Tspi_Context_FreeMemory(context, memory);
On 2011/03/29 19:16:06, gauravsh wrote:
> so in ScopedTssContextRelease, you check for (context) before calling close on
> it. Any particularly reason why you don't this here.
> 
> Same comments for ScopeTssObjectRelease below (check for context and handle)

For ContextRelease I assert it _is_ 0 and not that it isn't because  in that
case, we use the templated type for the context storage and not the context
itself.

However, I should be checking on the others ...

done!

http://codereview.chromium.org/6683071/diff/10002/tpm.cc
File tpm.cc (right):

http://codereview.chromium.org/6683071/diff/10002/tpm.cc#newcode319
tpm.cc:319: ScopedTssKey local_key_handle(context_handle);
On 2011/03/29 19:16:06, gauravsh wrote:
> It is a bit confusing due to different semantics of the Scoped* data
structures
> - i.e. ScopedTssKey does not behave like a TSS_HKEY, it behaves like
TSS_HKEY*.
> Which is fine but you may consider renaming it make it reflect that
> (ScopedTssMemoryPtr) or something.
> 

The problem is - it isn't a scoped pointer and naming it as such implies memory
handling that isn't happening.  If using operator* is too confusing, I can just
use get() and get_ptr().  If we want to actually allocate these 4 byte integer
handles on the heap, I could just use scoped_ptr_malloc<> with a custom release
class, but I didn't want to start allocating these on the heap if I could help
it.

What do you think? .get() and .pointer() or actually start some heap churn?

> Another point: context_handle is used in different parts of the file to mean 2
> different things (TSS_HCONTEXT) and ScopesTssContext. Since the semantics of
> them are different, this gets confusing (I initially thought you had made a
> mistake and it should be *context_handle as in the previous function). I
quickly
> realized that here context_handle is an actual HCONTEXT. Use different var
names
> to make that distinction clearer.

Yeah - I just kept the names that were there and made them scoped when I could.

http://codereview.chromium.org/6683071/diff/10002/tpm.cc#newcode322
tpm.cc:322: init_flags,
On 2011/03/29 19:16:06, gauravsh wrote:
> nit: indent is off

Done.

http://codereview.chromium.org/6683071/diff/10002/tpm.cc#newcode331
tpm.cc:331: TSS_TSPATTRIB_KEY_INFO,
On 2011/03/29 19:16:06, gauravsh wrote:
> nit: indent is off

Nope it appears to line up with the * above it.

http://codereview.chromium.org/6683071/diff/10002/tpm.cc#newcode393
tpm.cc:393: *local_key_handle,
On 2011/03/29 19:16:06, gauravsh wrote:
> nit: could move this to the line above.

Done.

I moved it down so they'd be all aligned.  But I don't care either way :).

http://codereview.chromium.org/6683071/diff/10002/tpm.cc#newcode403
tpm.cc:403: *local_key_handle,
On 2011/03/29 19:16:06, gauravsh wrote:
> nit: could move this to the line above.

Done.

Same here.

http://codereview.chromium.org/6683071/diff/10002/tpm.cc#newcode905
tpm.cc:905: static_cast<BYTE *>(
On 2011/03/29 19:16:06, gauravsh wrote:
> ahh, the joys of type casting.


Poor fes :,^|

[gauravsh, 2011-03-29 20:21:54]

http://codereview.chromium.org/6683071/diff/10002/scoped_tss_type.h
File scoped_tss_type.h (right):

http://codereview.chromium.org/6683071/diff/10002/scoped_tss_type.h#newcode9
scoped_tss_type.h:9: //  ScopedTssContext context;
On 2011/03/29 19:42:22, Will Drewry wrote:
> On 2011/03/29 19:16:06, gauravsh wrote:
> > context_handle
> 
> Ugh. I hate that variable name, but okay. :)

I just meant that you use context_handle later in this example, so be
consistent. :)

http://codereview.chromium.org/6683071/diff/10002/scoped_tss_type.h#newcode11
scoped_tss_type.h:11: //  if (!OpenAndConnectTpm(context_handle.get(), &result))
As I was saying, you use context_handle here, just make it consistent throughout
- either is fine.

http://codereview.chromium.org/6683071/diff/10002/tpm.cc
File tpm.cc (right):

http://codereview.chromium.org/6683071/diff/10002/tpm.cc#newcode319
tpm.cc:319: ScopedTssKey local_key_handle(context_handle);
On 2011/03/29 19:42:22, Will Drewry wrote:
> On 2011/03/29 19:16:06, gauravsh wrote:
> > It is a bit confusing due to different semantics of the Scoped* data
> structures
> > - i.e. ScopedTssKey does not behave like a TSS_HKEY, it behaves like
> TSS_HKEY*.
> > Which is fine but you may consider renaming it make it reflect that
> > (ScopedTssMemoryPtr) or something.
> > 
> 
> The problem is - it isn't a scoped pointer and naming it as such implies
memory
> handling that isn't happening.  If using operator* is too confusing, I can
just
> use get() and get_ptr().  If we want to actually allocate these 4 byte integer
> handles on the heap, I could just use scoped_ptr_malloc<> with a custom
release
> class, but I didn't want to start allocating these on the heap if I could help
> it.
> 
> What do you think? .get() and .pointer() or actually start some heap churn?

get() and pointer() sound reasonable to me. Better than the operator option,
IMO. I agree that treating them as pointer is probably not a good idea.

On the other hand, I think even if you decide to stick with the current
operation* and get() behavior, just changing the var names used throughout
should make this clear (i.e. when context_handle is a Scoped*, call it
context_handle_ptr, or something else since it has different use semantics than
the real TSS_HCONTEXT context_handle.). The actual type names can be kept as-is.

> 
> > Another point: context_handle is used in different parts of the file to mean
2
> > different things (TSS_HCONTEXT) and ScopesTssContext. Since the semantics of
> > them are different, this gets confusing (I initially thought you had made a
> > mistake and it should be *context_handle as in the previous function). I
> quickly
> > realized that here context_handle is an actual HCONTEXT. Use different var
> names
> > to make that distinction clearer.
> 
> Yeah - I just kept the names that were there and made them scoped when I
could.

[Will Drewry, 2011-03-29 20:34:51]

PTAL

This is an in-between approach where the ScopedTss* is usable in the normal
places where the TSS_* is used, but doesn't mask everything -- just typecasts. 
It's a bit magic-y, but so it scoped_ptr so I can go either way (adding a .get()
as well or just goin back and using _ptr on the var names).

thanks!

[gauravsh, 2011-03-29 20:52:44]

lgtm

http://codereview.chromium.org/6683071/diff/12002/tpm.cc
File tpm.cc (right):

http://codereview.chromium.org/6683071/diff/12002/tpm.cc#newcode350
tpm.cc:350: Tspi_Key_CreateKey(local_key_handle, srk_handle, 0))) {
nit: just indent this as you do the previous if's

http://codereview.chromium.org/6683071/diff/12002/tpm.cc#newcode612
tpm.cc:612: if (TPM_ERROR(result =
nit: indent this as you indent the others

http://codereview.chromium.org/6683071/diff/12002/tpm.cc#newcode648
tpm.cc:648: if (TPM_ERROR(local_result =
nit: indent this as you indent the others.

[Will Drewry, 2011-03-29 20:54:59]

done - thanks!

http://codereview.chromium.org/6683071/diff/12002/tpm.cc
File tpm.cc (right):

http://codereview.chromium.org/6683071/diff/12002/tpm.cc#newcode350
tpm.cc:350: Tspi_Key_CreateKey(local_key_handle, srk_handle, 0))) {
On 2011/03/29 20:52:44, gauravsh wrote:
> nit: just indent this as you do the previous if's

Done.

http://codereview.chromium.org/6683071/diff/12002/tpm.cc#newcode612
tpm.cc:612: if (TPM_ERROR(result =
On 2011/03/29 20:52:44, gauravsh wrote:
> nit: indent this as you indent the others

it is - i think it's another rietveld optical illusion!

http://codereview.chromium.org/6683071/diff/12002/tpm.cc#newcode648
tpm.cc:648: if (TPM_ERROR(local_result =
On 2011/03/29 20:52:44, gauravsh wrote:
> nit: indent this as you indent the others.

Same here!

[gauravsh, 2011-03-29 20:58:45]

http://codereview.chromium.org/6683071/diff/12002/tpm.cc
File tpm.cc (right):

http://codereview.chromium.org/6683071/diff/12002/tpm.cc#newcode612
tpm.cc:612: if (TPM_ERROR(result =
On 2011/03/29 20:54:59, Will Drewry wrote:
> On 2011/03/29 20:52:44, gauravsh wrote:
> > nit: indent this as you indent the others
> 
> it is - i think it's another rietveld optical illusion!

It is? All the red and green (the CL, not the tree) must be making me lose my
mind. (BTW, I meant indent like this: 

if (TPM_ERROR(result = Tspi_Context_GetTpmObject(context_handle,
                                                 &tpm_handle)))...
)

[Will Drewry, 2011-03-29 21:14:30]

ah! fixed!

http://codereview.chromium.org/6683071/diff/12002/tpm.cc
File tpm.cc (right):

http://codereview.chromium.org/6683071/diff/12002/tpm.cc#newcode350
tpm.cc:350: Tspi_Key_CreateKey(local_key_handle, srk_handle, 0))) {
On 2011/03/29 20:52:44, gauravsh wrote:
> nit: just indent this as you do the previous if's

Done.

http://codereview.chromium.org/6683071/diff/12002/tpm.cc#newcode612
tpm.cc:612: if (TPM_ERROR(result =
On 2011/03/29 20:58:45, gauravsh wrote:
> On 2011/03/29 20:54:59, Will Drewry wrote:
> > On 2011/03/29 20:52:44, gauravsh wrote:
> > > nit: indent this as you indent the others
> > 
> > it is - i think it's another rietveld optical illusion!
> 
> It is? All the red and green (the CL, not the tree) must be making me lose my
> mind. (BTW, I meant indent like this: 
> 
> if (TPM_ERROR(result = Tspi_Context_GetTpmObject(context_handle,
>                                                  &tpm_handle)))...
> )

Done.

http://codereview.chromium.org/6683071/diff/12002/tpm.cc#newcode648
tpm.cc:648: if (TPM_ERROR(local_result =
On 2011/03/29 20:54:59, Will Drewry wrote:
> On 2011/03/29 20:52:44, gauravsh wrote:
> > nit: indent this as you indent the others.
> 
> Same here!

Done.

[fes, 2011-03-29 22:55:38]

LGTM other than minor nits.

http://codereview.chromium.org/6683071/diff/6007/scoped_tss_type.h
File scoped_tss_type.h (right):

http://codereview.chromium.org/6683071/diff/6007/scoped_tss_type.h#newcode65
scoped_tss_type.h:65: explicit ScopedTssType(TSS_HCONTEXT c = 0, TssType t = 0)
:
Colon goes on the next line

http://codereview.chromium.org/6683071/diff/6007/tpm.cc
File tpm.cc (right):

http://codereview.chromium.org/6683071/diff/6007/tpm.cc#newcode642
tpm.cc:642: local_context_handle.ptr()))) {
Spacing is off.