GetTokenInformation example done right

[Joseph M. Newcomer]

In another newsgroup, someone posted a piece of code that did some of most bizarre and
convoluted allocation I had seen in a while. When I asked "why use HeapAlloc?", I was
pointed to a Microsoft example called "Getting the login SID in C++". It is abysmal C++
programming, and poor C programming. I have critiqued this piece of trash and rewritten
it in both MFC and C so it looks like it was written by someone who actually *learned*
modern C or C++, and didn't have their education in C stop with the K&R C book. Code like
this was bad code in 1975, when K&R C was the only version available, and 33 years later
it is simply unacceptable that anyone would write code this poor, and foist it on an
unsuspecting public as if it were a good example of how to write code.

http://www.flounder.com/msdn_documentation_errors_and_omissions.htm#GetTokenInformation

I have a <TODO> for an example with smart pointers, because I didn't have time to deal
with the smart pointer example (I only have 2008 on my laptop, and I'm sitting at my
desktop today); if someone wants to send me a 2008 Feature Pack shared_ptr example line or
three to put there, I'll be more than happy to give you a credit line...
joe
Joseph M. Newcomer [MVP]
email: newc...@flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm

[Giovanni Dicanio]

"Joseph M. Newcomer" <newc...@flounder.com> ha scritto nel messaggio
news:675e9419nipjura4f...@4ax.com...

> http://www.flounder.com/msdn_documentation_errors_and_omissions.htm#GetTokenInformation
>
> I have a <TODO> for an example with smart pointers, because I didn't have
> time to deal
> with the smart pointer example (I only have 2008 on my laptop, and I'm
> sitting at my
> desktop today); if someone wants to send me a 2008 Feature Pack shared_ptr
> example line or
> three to put there, I'll be more than happy to give you a credit line...

Hi Joe,

I think that there is a typo in your code:

<code>
std::vector<BYTE> ptgbuffer;
ptgbuffer.resize(dwLength);
PTOKEN_GROUPS ptg = &ptgbuffer[0];
</code>

You need a cast there:

PTOKEN_GROUPS ptg = (PTOKEN_GROUPS) &(ptgBuffer[0]);

or if you like C++ style casts:

PTOKEN_GROUPS ptg = reinterpret_cast< PTOKEN_GROUPS >( &(ptgBuffer[0]) );

I don't understand why you want a shared_ptr there... I think std::vector is
just fine.

Giovanni

[Joseph M. Newcomer]

Oops! Fixed, thanks.

I wanted the shared_ptr for completeness. I would use the CByteArray solution in MFC and
the std::vector solution for non-MFC C++.
joe
On Mon, 4 Aug 2008 17:12:17 +0200, "Giovanni Dicanio" <gdicanio@_NOSPAM_email_DOT_it>
wrote:

[Giovanni Dicanio]

"Joseph M. Newcomer" <newc...@flounder.com> ha scritto nel messaggio

news:hi7e94lj3i2nscb4k...@4ax.com...

> I wanted the shared_ptr for completeness.

I would use shared_ptr if there was an instance of some class, e.g.

// class TokenGroup
shared_ptr< TokenGroup > tokenGroup( new TokenGroup(...) );

However, there you have a length in bytes (dwLength), and you need to
allocate a raw buffer to store an array of TOKEN_GROUPS.
So, why the need of shared_ptr?

BTW: I did not read *all* your code, so maybe I'm missing something.

Thanks,
Giovanni

[Giovanni Dicanio]

Joe, just one more little thing:

In paragraph "Rewritten in C++/MFC" you have:

<code>
psid = (PSID) new BYTE[dwLength];
if (psid == NULL)
return FALSE;
</code>

I think that 'new' returns NULL on failure only in VC6.
Since VC7.1 'new' behaves like the C++ standard says, so it throws a
std::bad_alloc exception and does not return NULL on error.

(Please correct if I'm wrong.)

Giovanni

"Joseph M. Newcomer" <newc...@flounder.com> ha scritto nel messaggio

news:675e9419nipjura4f...@4ax.com...

[Giovanni Dicanio]

My last question:

Why do you use __try?
I would use C++ standard try instead...

Or do you want to control exceptions that 'try' can't catch, but __try can?

Thanks
Giovanni

"Joseph M. Newcomer" <newc...@flounder.com> ha scritto nel messaggio
news:675e9419nipjura4f...@4ax.com...

[Joseph M. Newcomer]

Well, the title of that section was "But what if I needed it in C?" We don't need
try/catch in C++ because the automatic destructors handle it anyway, but in C, the excuse
is "but you need that goto" (I've heard this far too often). So in C, since we don't have
try/catch or destructors, I chose to use the C extension __try/__catch. This is why I
said that even as C code it was poor code as written.
joe

On Mon, 4 Aug 2008 17:36:25 +0200, "Giovanni Dicanio" <gdicanio@_NOSPAM_email_DOT_it>
wrote:

>My last question:

[Giovanni Dicanio]

"Joseph M. Newcomer" <newc...@flounder.com> ha scritto nel messaggio

news:je9e941b3sd1fl5ap...@4ax.com...

> Well, the title of that section was "But what if I needed it in C?" We
> don't need
> try/catch in C++ because the automatic destructors handle it anyway, but
> in C, the excuse
> is "but you need that goto" (I've heard this far too often). So in C,
> since we don't have
> try/catch or destructors, I chose to use the C extension __try/__catch.
> This is why I
> said that even as C code it was poor code as written.

Thank you! Now I understand: you needed an exception mechanism for pure C
(no C++), and __try/__catch are available in C.
I think it is a Microsoft extension, but useful to avoid "goto cleanup;".
In fact, if I had used pure C, I would have used the "goto cleanup;"... (I
was ignoring the existance __try/__catch in C...).

Giovanni

[Joseph M. Newcomer]

Actually, this is not always true. When using mfc, you get the operator new in
afxmem.cpp:

from afxmem.cpp:

void* __cdecl operator new(size_t nSize, int nType, LPCSTR lpszFileName, int nLine)
{
#ifdef _AFX_NO_DEBUG_CRT
UNUSED_ALWAYS(nType);
UNUSED_ALWAYS(lpszFileName);
UNUSED_ALWAYS(nLine);
return ::operator new(nSize);
#else
void* pResult;
#ifdef _AFXDLL
_PNH pfnNewHandler = _pfnUninitialized;
#endif
for (;;)
{
pResult = _malloc_dbg(nSize, nType, lpszFileName, nLine);
if (pResult != NULL)
return pResult;

so it still runs as it did before, returning NULL (I got this by creating an MFC app,
doing new BYTE[1024], and single-stepping into it.)

But in a non-MFC app, in new.cpp, it is

void *__CRTDECL operator new(size_t size) _THROW1(_STD bad_alloc)
{ // try to allocate size bytes
void *p;
while ((p = malloc(size)) == 0)
if (_callnewh(size) == 0)
{ // report no memory
static const std::bad_alloc nomem;
_RAISE(nomem);
}

return (p);
}

So the code shown depends on whether or not this C++ example is executed in the context of
MFC or the context of C++, which is a real unfortunate glitch.
joe

On Mon, 4 Aug 2008 17:33:14 +0200, "Giovanni Dicanio" <gdicanio@_NOSPAM_email_DOT_it>
wrote:

>Joe, just one more little thing:

[Joseph M. Newcomer]

Check out the rewrite...
joe

On Mon, 4 Aug 2008 17:33:14 +0200, "Giovanni Dicanio" <gdicanio@_NOSPAM_email_DOT_it>
wrote:

>Joe, just one more little thing:

[Alf P. Steinbach]

* 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?

[Joseph M. Newcomer]

Thanks. See below...

****
MOstly because this was intended to be an MFC example, and mixing BOOL and bool is
confusing and potentially dangerous
****

>
>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.

*****
I have no argument with that.n I was just taking the existing example and correcting its
more egregious failures.
****

>
>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. ;-)

*****
I developed the habit early on, and see no reason to change it. People can use whatever
indentation style they like, but when I write code, it uses my style. I've been using it
for about 40 years.
*****

>
>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! :-)

*****
I was suspicious of that myself, because it seemed to me that if I just asked for the
buffer size it should give it to me successfully, but some of the APIs will return an
error code indicating the 0-sized buffer was too small; I presumed that this API was
determined to be one of the highly-idiosyncratic ones. It isn't helped by the fact that
virtually no API ever says what error codes it can return or what the causes might be!
*****

>
>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!

*****
The only reason I left the LPVOID casts in is that under some scenarios (such as lint,
possibly prefast and possibly under /W4, failure to do an (LPVOID) cast and relying on the
implicit "superclass widening" of the pointer sometimes generates (spurious) warning
messages about type mismatches. Short of running lint and prefast on the example, I
wasn't sure if that was the purpose of these casts or not; it certainly struck me that a
programmer who could write code this bad probably added gratuitous casts.
*****

>
>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!

*****
Actually, check my rewrite. new BYTE[n] returns NULL in MFC, and throws std::bad_alloc if
MFC is not being used. Inconsistent and confusing.
*****

*****
Could I publish this rewrite as part of the article? In practice, I tend to refactor much
as you have done here. I just didn't feel up to doing all the work, just to show that the
original code sucked to the millimicron vacuum gauge level...

Of course, you'd get a credit line...
joe
*****

[Joseph M. Newcomer]

In C, it is __try/__except, and the exception clause doesn't resemble a catch clause. But
I used something called a "termination block" which is __try/__finally, and essentially
the __finally says that it is impossible to leave (via return, break, goto, or any other
control transfer) the protected block without executing the __finally clause to do the
cleanup

What is odd is that if you do

void whatever(parms)
{
CString t;
CString s;
__try {
...
}
__finally{
}
}
with no C++ objects in the scope of the __try, the compiler takes a fatal error and says
you cannot mix C++ objects requiring destructors with C exceptions, even though that has
nothing to do with what was actually coded above. It is a case of overcompensation
created by a failure to analyze the code correctly. Since there are no C++ objects
inside, and all paths lead to __finally, by the time __finally does something (including
return, break, etc.) we are back into the C++ scope world. I wish they would fix this
compiler bug. While it would make sense for __try/__except and it would make sense if
there were C++ objects declared in the scope of the __try, it makes no sense in the above
example.
joe

On Mon, 4 Aug 2008 18:05:14 +0200, "Giovanni Dicanio" <gdicanio@_NOSPAM_email_DOT_it>
wrote:

>

[Alf P. Steinbach]

* Joseph M. Newcomer:

> Could I publish this rewrite as part of the article? In practice, I tend to refactor much
> as you have done here. I just didn't feel up to doing all the work, just to show that the
> original code sucked to the millimicron vacuum gauge level...
>
> Of course, you'd get a credit line...

Why, you're welcome.

But please note somewhere that it's almost off-the-cuff code, not tested except
to see that it "worked".

I find almost always that when code is put to work for real, in applications,
then one uncovers limitations, bugs, bad design, redundant functionality, etc.
etc., because it's just plain impossible to think of everything up front.

Cheers,

[Joseph M. Newcomer]

Your refactoring is now included in the discussion.
thanks
joe

On Mon, 04 Aug 2008 22:52:15 +0200, "Alf P. Steinbach" <al...@start.no> wrote:

>* Joseph M. Newcomer:
>> Could I publish this rewrite as part of the article? In practice, I tend to refactor much
>> as you have done here. I just didn't feel up to doing all the work, just to show that the
>> original code sucked to the millimicron vacuum gauge level...
>>
>> Of course, you'd get a credit line...
>
>Why, you're welcome.
>
>But please note somewhere that it's almost off-the-cuff code, not tested except
>to see that it "worked".
>
>I find almost always that when code is put to work for real, in applications,
>then one uncovers limitations, bugs, bad design, redundant functionality, etc.
>etc., because it's just plain impossible to think of everything up front.
>
>
>Cheers,
>
>- Alf

[Doug Harrison [MVP]]

On Mon, 04 Aug 2008 16:14:41 -0400, Joseph M. Newcomer
<newc...@flounder.com> wrote:

>Actually, check my rewrite. new BYTE[n] returns NULL in MFC, and throws std::bad_alloc if
>MFC is not being used.

Actually, it throws CMemoryException* in MFC programs.

>Inconsistent and confusing.

True, and too bad for non-MFC code that specifically catches
std::bad_alloc. That said, MFC predates std::bad_alloc by several years,
and I don't see how they could have changed things. Indeed, MFC is
completely unaware of any exception type other than CException*, which I
talked about here:

Q6. How do I safely use _com_error, std::exception, and other non-MFC
exception classes in MFC programs?
http://members.cox.net/doug_web/eh.htm#Q6

--
Doug Harrison
Visual C++ MVP

[Joseph M. Newcomer]

Well, I single-stepped through the code, and it returns NULL. That's why I showed the
code.
joe

[Giovanni Dicanio]

"Joseph M. Newcomer" <newc...@flounder.com> ha scritto nel messaggio

news:gdce94dbu9im5ko6t...@4ax.com...

> Check out the rewrite...
> joe

Thanks Joe! :)

Giovanni

[Giovanni Dicanio]

One more little "critique" to Alf's code :)

I think this function is not good, because it is ANSI/MBCS only, I think it
won't compile in Unicode:

<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>

This is because ConvertSidToStringSid function has this prototype:

BOOL ConvertSidToStringSid(
__in PSID Sid,
__out LPTSTR *StringSid
);

instead in your code you use std::string, which is based on char* (no
TCHAR).

A CString should be better used, or if one likes the STL string classes,
better doing the usual tip-and-trick:

typedef std::basic_string< TCHAR > tstring;

Giovanni

[Giovanni Dicanio]

"Joseph M. Newcomer" <newc...@flounder.com> ha scritto nel messaggio

news:je9f94peg11klj85f...@4ax.com...

> Well, I single-stepped through the code, and it returns NULL. That's why
> I showed the
> code.

I read the documentation of AfxThrowMemoryException, and it says:

"You do not need to call it for new because new will throw a memory
exception automatically if the memory allocation fails."

...very confusing.

So, what is the truth? It seems that official documentation and Doug agree
that new throws CMemoryException on bad allocation... (but that is not very
clear for single-stepped code...).

Giovanni

[Alf P. Steinbach]

* Giovanni Dicanio:

Well, I don't think Windows 9x compatibility (T) is very important here. After
all, at least AFAIK, many of the relevant functions are only implemented on NT
platforms. So for Unicode I'd just use a std::wstring, but still narrow
character strings for the exceptions.

That means an additional signature, perhaps a name change, and that the above
(since Unicode is the only reasonable default) should better use
ConvertSidToStringSidA, assuming that's the narrow character version's name.

Sorry for writing that code in haste. :-)

And thanks for pointing that out.

I was mostly focusing on refactoring the original code.

[Joseph M. Newcomer]

Missed it. Yes, it does thow CMemoryException. I hadn't stepped far enough into it (it
is handled by the callback handler)
joe

On Tue, 5 Aug 2008 12:20:07 +0200, "Giovanni Dicanio" <gdicanio@_NOSPAM_email_DOT_it>
wrote:

>

[Joseph M. Newcomer]

Thanks to all, there's yet a newer revision out there.
joe

On Tue, 5 Aug 2008 12:20:07 +0200, "Giovanni Dicanio" <gdicanio@_NOSPAM_email_DOT_it>
wrote:

>

[David Ching]

"Joseph M. Newcomer" <newc...@flounder.com> wrote in message
news:u9hg94hbc19ci5kr7...@4ax.com...

> Thanks to all, there's yet a newer revision out there.

I'm wondering how good the original MSDN doc would have been had it been
subjected to the open community for critiquing! ;)

-- David

[Giovanni Dicanio]

"Alf P. Steinbach" <al...@start.no> ha scritto nel messaggio
news:KLudnTfwQsiWsgXV...@posted.comnet...

> Well, I don't think Windows 9x compatibility (T) is very important here.
> After all, at least AFAIK, many of the relevant functions are only
> implemented on NT platforms. So for Unicode I'd just use a std::wstring,
> but still narrow character strings for the exceptions.

I agree with you about Win9x compatibility.
However, we get the "T" stuff for free if we use CString (but I recall that
you like STL string class from some other posts of your :)
So, if one should use STL string, then OK std::wstring is fine.

> Sorry for writing that code in haste. :-)

No need to be sorry :)

You showed important refactor in your code, and that is the really important
thing.

Giovanni

[Giovanni Dicanio]

"David Ching" <d...@remove-this.dcsoft.com> ha scritto nel messaggio
news:ZuYlk.18201$89.1...@nlpi069.nbdc.sbc.com...

David: more minds work better than one single mind, I think! :)

Someone sees some thing, someone other sees some other thing, etc.
The final result tends to be better thanks to the contributions of several
minds.

(I think that this is one of the principles that justifies the good quality
of some open source software, where the software source code is subjected to
the analysis, "critiquing" and improvment done by lots of people in the
community.)

G

[Les]

One of the benfits I derive from this NG is not only learning how to do
something but *why* it is good practice to do it that way.
After learning the hard way not to trust the MSDN examples (surely these are
the people who put the "M" into MSDN they must know what they are doing
Hah!) I went back and re-worked the code I had written, based on what I saw
as I lurked here.
A prime example (for me) being GetDlgItem which occured in code I used that
had been written by previous programmers so I thought they knew what they
were doing :-)

Les

"Giovanni Dicanio" <gdicanio@_NOSPAM_email_DOT_it> wrote in message
news:ORJKlRw9...@TK2MSFTNGP05.phx.gbl...