Re: GetTokenInformation example done right

* Joseph M. Newcomer:


Ah well. I think your attempt to expose Bad Programming(TM) in MSDN is laudable,
good, because many novices and even professionals take these examples as being
examples of good ways to do things. Which they decidedly are not, in general.

But let's critique your code also. ;-)

At the end I supply some alternative C++ code, which I expect you'll then
analyze to death...

<code author="joe">
BOOL GetLogonSID (HANDLE hToken, PSID & psid)
    {
     DWORD dwLength = 0;
     CByteArray b;

     // Get required buffer size and allocate the TOKEN_GROUPS buffer.

     if (!GetTokenInformation(
              hToken, // handle to the access token
              TokenGroups, // get information about the token's groups
              (LPVOID) ptg, // pointer to TOKEN_GROUPS buffer
              0, // size of buffer
              &dwLength // receives required buffer size
              ))
        {
         if (GetLastError() != ERROR_INSUFFICIENT_BUFFER)
            return FALSE;

         b.SetSize(dwLength);
        }

     PTOKEN_GROUPS ptg = b.GetData();

     // Get the token group information from the access token.

     if (!GetTokenInformation(
              hToken, // handle to the access token
              TokenGroups, // get information about the token's groups
              (LPVOID) ptg, // pointer to TOKEN_GROUPS buffer
              dwLength, // size of buffer
              &dwLength // receives required buffer size
              ))
         {
          return FALSE;

         }

     // Loop through the groups to find the logon SID.

     for (DWORD dwIndex = 0; dwIndex < ptg->GroupCount; dwIndex++)

        if ((ptg->Groups[dwIndex].Attributes & SE_GROUP_LOGON_ID)
              == SE_GROUP_LOGON_ID)
          {
           // Found the logon SID; make a copy of it.

           dwLength = GetLengthSid(ptg->Groups[dwIndex].Sid);
           psid = (PSID) new BYTE[dwLength];

           if (psid == NULL)
              return FALSE;

           if (!CopySid(dwLength, *ppsid, ptg->Groups[dwIndex].Sid))
              {
               delete [] psid;
               psid = NULL;

               return FALSE;
              }
           break;

          }

      return TRUE;

     }
</code>

The first thing that hit me was the 'BOOL' result type. Why not use standard C++
'bool'? For that matter, I think it would be more practical with an exception on
error, and simply 'void' result type, but that's very much personal preference.

Next, that it is a single *routine* (OK, function). There's a lot of
functionality in here. Much of it will have to be duplicated elsewhere when it's
stuffed into a routine like this. Separating out this functionality will however
produce more lines of code (example below). But I think it's worth it, because
reuse is not only desirable on its own, it's also very desirable for testing and
to be a bit more sure about correctness.

Next, the line after the function head, indentation of the {. I used to drive my
co-workers mad by indenting like that, once upon a time. Got that from early
student work implementing compiler and indentation "should" match parsing level,
I thought, and besides, Petzold did it that way. But it is about communicating
to other programmers, not just to oneself. So I gave up that style. ;-)

Next, we have that "if( !GetTokenInformation ..." for checking buffer length.
I'm reasonably sure that you didn't invent this idiocy, but simply didn't
recognize it as such in the original MSDN Library code. It is pure idiocy. For
if GetTokenInformation returns logical false, then all is OK, and the if is
unnecessary. But if against expectation GetTokenInformation returns logical
true, then, hey, this code isn't executed, and you're into UB (or at least very
unexpected and not-catered-for) land! Ouch! :-)

What's needed, if any result checking is to be done, is not an 'if', but an
'assert': asserting that with these arguments, GetTokenInformation returns
logical false, and if it doesn't then something is Very Very Wrong(TM). Btw.,
regarding the arguments, the ptg argument is unnecessary here (simply use 0),
and in addition that cast to 'LPVOID' is unnecessary, and in addition if there
should be a cast it should be a static_cast and to 'void*'. Again, this stems
from the original MSDN Library code. I'm very very sure it isn't intentional!

Next, further down there is a check "if (psid == NULL)". Well, if this code is
supporting VC6, then OK, it might happen. But I'd put a comment on that. In
modern C++ 'new' throws a std::bad_alloc exception on failure. By the way, for
the cleanup below that check (after CopySid), I suggest looking at Marginean &
Alexandrescu's ScopeGuard. It needs some adjustment for Visual C++ (because MSVC
is defective re __LINE__ macro, so instead use __COUNTER__), but very useful!

OK, over to what I suggested regarding refactoring.

In this code I use some very primitive exception throwing. It's ungood because
it doesn't pick up e.g. GetLastError, but it's simple, so, I use it for examples
like this, & simple test code. Goes like (Visual C++ specific code):

<code>
#pragma warning( disable: 4646 ) // noreturn func has non-void result type
__declspec( noreturn )
bool throwX( char const s[] ) { throw std::runtime_error( s ); }
</code>

As an example usage,

<code>
std::string stringFromSid( PSID sid )
{
     char* p;
     ConvertSidToStringSid( sid, &p )
         || throwX( "stringFromSid: ConvertSidToStringSid failed" );
     std::string const result = p;
     LocalFree( p );
     return result;
}
</code>

Next, primitive API-wrapper level (I had to reformat for posting, hope that
hasn't introduced any erors),

<code>
typedef HANDLE Token;
typedef TOKEN_INFORMATION_CLASS TokenInfoClassEnum;

DWORD tokenInfoLength( Token token, TokenInfoClassEnum infoClass )
{
     DWORD length;
     BOOL const ok = GetTokenInformation( token, infoClass, 0, 0, &length );

     assert( !ok );
     (GetLastError() == ERROR_INSUFFICIENT_BUFFER)
         || throwX( "tokenInfoLength: GetTokenInformation failed" );
     return length;
}

void getTokenInfo(
     Token token,
     TokenInfoClassEnum infoClass,
     std::vector<char>& result
     )
{
     std::vector<char> resultData( tokenInfoLength( token, infoClass ) );
     DWORD dummy;
     BOOL const ok = GetTokenInformation(
         token, infoClass, &resultData[0],
         static_cast<DWORD>( resultData.size() ), &dummy
         );

     (ok) || throwX( "tokenInfo: GetTokenInformation failed" );
     resultData.swap( result );
}
</code>

Here's one important aspect, that 'getTokenInfo' *cannot* be reimplemented as or
wrapped by an 'tokenInfo' function returning std::vector<char>.

The reason it cannot, is that GetTokenInformation places SID data in the buffer,
and it places pointers to those SIDs in the buffer. Copying and destroying
original buffer would trash those embedded pointers.

At a slightly higher level of abstraction, factoring out the "get group info"
functionality, that non-copyability can be enforced in C++:

<code>
class AttributedSids
{
private:
     std::vector<char> myData;
     TOKEN_GROUPS const* myDataPtr;

     AttributedSids( AttributedSids const& );
     AttributedSids& operator=( AttributedSids const& );

public:
     typedef SID_AND_ATTRIBUTES AttributedSid;

     AttributedSids( Token token )
     {
         getTokenInfo( token, TokenGroups, myData );
         myDataPtr = reinterpret_cast<TOKEN_GROUPS const*>( &myData[0] );
     }

     size_t size() const { return myDataPtr->GroupCount; }

     AttributedSid const& operator[]( size_t i ) const
     {
         return myDataPtr->Groups[i];
     }

     AttributedSids const& ref() const { return *this; }
};
</code>

But then, it might be useful to really copy SIDs, so, a class for that:

<code>
class Sid
{
private:
     std::vector<char> myData;

protected:
     Sid( PSID aSid )
     {
         myData.resize( ::GetLengthSid( aSid ) );
         ::CopySid( static_cast<DWORD>( myData.size() ), &myData[0], aSid )
             || throwX( "Sid::<init>: CopySid failed" );
     }

public:
     operator PSID () const { return const_cast<char*>( &myData[0] ); }
     std::string toString() const { return stringFromSid( *this ); }
};
</code>

Probably that constructor should be public for a reusable class, but I chose the
least access that would work for this example, namely, for derived class that
provides logon SID:

<code>
class LogonSid: public Sid
{
public:
     static PSID ptrIn( AttributedSids const& sids )
     {
         for( size_t i = 0; i < sids.size(); ++i )
         {
             if( (sids[i].Attributes & SE_GROUP_LOGON_ID) == SE_GROUP_LOGON_ID )
             {
                 return sids[i].Sid;
             }
         }
         throwX( "LogonSid::ptrIn: logon SID not found" );
     }

     LogonSid( HANDLE token )
         : Sid( LogonSid::ptrIn( AttributedSids( token ).ref() ) )
     {}
};
</code>

Now if I were to critique this code myself I would probably focus on that
constructor initializer line, because it uses some non-obvious C++ features and
rules. But I'm not sure how to do that better. Essentially, the problem is
keeping a copy of the SID data until base class has been initialized, and the
non-obvious rule is that the *current* C++ standard doesn't support binding a
temporary of non-copyable class to a ref-to-const formal arg. An alternative
could be to introduce a dependency on AttributedSids in the Sid base class. But
I think that's even uglier, but possibly someone here has Bright Idea?

Cheers, & hth.,

- Alf

--
A: Because it messes up the order in which people normally read text.
Q: Why is it such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?