private destructor and templates

[Ben Voigt]

I'm passing information between threads (one managed, one not) and if

That is in violation of the docs for PostMessage:
"If you send a message in the range below WM_USER to the asynchronous
message functions (PostMessage, SendNotifyMessage, and
SendMessageCallback), its message parameters cannot include pointers.
Otherwise, the operation will fail. The functions will return before the
receiving thread has had a chance to process the message and the sender
will free the memory before it is used."

My message isn't below WM_USER. It is however, exactly equal to WM_USER,
which raises the concern that some other app might also use it, and might
not be careful where it sends messages.

In other words, you shouldn't fire off pointers to a window and expect the
window to delete them. Instead, you should use SendMessage, and delete
them yourself. This then avoids the whole issue.

Since I'm implementing a user message where I control both ends, having the
window delete the pointers is completely feasible. SendMessage would not
be, since it would block my worker thread.

I don't understand that.

(*(LPFUNCTION)lParam)() or ((CSomething*)lParam)->method() with method being
virtual, is a huge security hole.

((CSomething*)lParam)->method() where method is non-virtual is quite safe
however, provided method doesn't implicitly trust its this pointer. Since I
am only reading from the structure and have that wrapped in an SEH block, I
think I'm ok. The idea of freeing that pointer still troubles me somewhat.
A message authentication code of some type might be in order.

Sorry, I've no .NET expertise.

The issue is that DispatchMessage handles WM_TIMER in exactly the broken
method described above.

Ahh, ok.


Ahh, yes. I didn't realise the derived classes have destructors (which is
yet another violation of the requirements on POD data).

I made operator delete protected before deleting the private destructor,
which certainly contributed to the problem. But all of my constructors
needed to be declared non-throwing anyway.

Calling delete with the wrong type is illegal C++, unless the destructor
is virtual. It happens to work on some compilers and versions, but not
necessarily all, since the compiler is at liberty to use the type of the
deleted object to determine the size of the allocation that created it.
Also, a debugging implementation might check that constructed objects have
their destructors run.

Based on what you've said, the easiest option would be to stop using
PostMessage and instead destroy messages at the sending site. If you

Can't do that, the message passing vs remote procedure call distinction is
important here.

can't do that, you are probably best off moving to using real POD objects,
dropping your use of new and delete expressions, and using :perator new
and :perator delete (or malloc and free), which work

I'd defined my own operator new and operator delete in the base class as
you'll recall. I had used
static void* operator new(size_t bytes)
{
return new char[bytes];
}

static void operator delete(void* p)
{
delete [] p;
}

I'll go ahead and change these to call malloc and free. Would that be legal
then?

Some derived classes do override operator new with a placement parameter
indicating the size of an variably-sized buffer. They delegate to the base
class operator new after adjusting the size of the allocation. Is that also
highly suspect?

fine with raw memory, and don't care about pointer types or destructors.
If you want more standards compliant code, you could replace constructors
with initialization functions, and drop inheritence entirely (making your
Destroy function a free function).

I could do that by making the first member of each structure an instance of
what's currently the base class, in the same fashion that the Windows API
does structure polymorphism. But what would that change, besides having to
explicitly specify the name of that structure to use its members?

I take it you are suggesting allocating structures with
(derived_type*) base_type::Alloc(sizeof derived_type + extra_buffer_bytes)

It seems to me that you're conflating OO and C style programming in an
unnecessary and confusing way.

Would you prefer I use a POD union? That has really bad locality of
definition which translates into unmaintainability.

C style programming can be object oriented, and C++ can be procedural.
Really C++ is just a lot of syntactic sugar on top of C, plus some really
nifty things like templates.

I find it really handy to have a polymorphic set of structures that share
the initial few members and then each add several more. This is possible in
C, but really ugly. Take a look at any code using BITMAPCOREHEADER and it's
derivatives BITMAPINFOHEADER, BITMAPV4HEADER, BITMAPV5HEADER in the context
of BITMAPINFO and you'll see the ugliness I'm trying to avoid.

I'm sorry to hear that what I'm doing is broken, because maintaining
duplicate code ala BITMAP*HEADER isn't very appealing (did we really need
four different declarations of the width member with four different names?).

 

[Tom Widmer [VC++ MVP]]

My message isn't below WM_USER. It is however, exactly equal to WM_USER,
which raises the concern that some other app might also use it, and might
not be careful where it sends messages.


Since I'm implementing a user message where I control both ends, having the
window delete the pointers is completely feasible. SendMessage would not
be, since it would block my worker thread.

A strict reading of the PostMessage docs for VC2005 implies it is
illegal, though I don't understand why it should be illegal, since, as
you say, you have control of the pointers at both ends.

(*(LPFUNCTION)lParam)() or ((CSomething*)lParam)->method() with method being
virtual, is a huge security hole.

((CSomething*)lParam)->method() where method is non-virtual is quite safe
however, provided method doesn't implicitly trust its this pointer. Since I
am only reading from the structure and have that wrapped in an SEH block, I
think I'm ok. The idea of freeing that pointer still troubles me somewhat.
A message authentication code of some type might be in order.

I see, that seems sensible to avoid.

Sorry, I meant constructors!

I made operator delete protected before deleting the private destructor,
which certainly contributed to the problem. But all of my constructors
needed to be declared non-throwing anyway.

Calling delete with the wrong type is illegal C++, unless the destructor
is virtual. It happens to work on some compilers and versions, but not
necessarily all, since the compiler is at liberty to use the type of the
deleted object to determine the size of the allocation that created it.
Also, a debugging implementation might check that constructed objects have
their destructors run.

Based on what you've said, the easiest option would be to stop using
PostMessage and instead destroy messages at the sending site. If you

Can't do that, the message passing vs remote procedure call distinction is
important here.

can't do that, you are probably best off moving to using real POD objects,
dropping your use of new and delete expressions, and using :perator new
and :perator delete (or malloc and free), which work

I'd defined my own operator new and operator delete in the base class as
you'll recall. I had used
static void* operator new(size_t bytes)
{
return new char[bytes];
}

static void operator delete(void* p)
{
delete [] p;
}

I'll go ahead and change these to call malloc and free. Would that be legal
then?

An alternative fix is:
static void operator delete(void* p)
{
delete [] static_cast<char*>(p);
}

The point is, you should not be calling constructors if you don't match
the constructor calls with equivalent destructor calls. To avoid
constructor calls, you shouldn't use new-expressions.

Some derived classes do override operator new with a placement parameter
indicating the size of an variably-sized buffer. They delegate to the base
class operator new after adjusting the size of the allocation. Is that also
highly suspect?


I could do that by making the first member of each structure an instance of
what's currently the base class, in the same fashion that the Windows API
does structure polymorphism. But what would that change, besides having to
explicitly specify the name of that structure to use its members?

Well, there are guarantees regarding the layout of members of PODs. With
derivation, there is no guarantee. For example, derived class members
might be before or after base class members in memory. At the extreme,
base class members could be referenced in the derived class by a
pointer! e.g.

struct Base
{
int i;
};

class Derived: public Base
{
int derivedMember1;
Base* __hiddenBasePointer;

void f()
{
i = 10;
//translates to __hiddenBasePointer->i = 10;
}
};

Obviously, the implicitly generated constructors and destructors would
have to handle failure proof allocating and deleting of the pointer.

Note I don't know any compiler that does this in practice! I'm just
highlighting the lack of guarantees you get when you switch to using
derivation.

I take it you are suggesting allocating structures with
(derived_type*) base_type::Alloc(sizeof derived_type + extra_buffer_bytes)

Well, I'd wrap that in a call:

derived_type* p = derived_type::Alloc();
//...
base_type::Free(p);

Using the curiously recurring template pattern, you could even avoid
having to write the Alloc function in each derived class.

Would you prefer I use a POD union? That has really bad locality of
definition which translates into unmaintainability.

No, I don't think that would be better.

C style programming can be object oriented, and C++ can be procedural.
Really C++ is just a lot of syntactic sugar on top of C, plus some really
nifty things like templates.

Right, I suppose I mean that if you don't want to go the C++ OO route,
for whatever reason, you should go the C OO route.

I find it really handy to have a polymorphic set of structures that share
the initial few members and then each add several more. This is possible in
C, but really ugly. Take a look at any code using BITMAPCOREHEADER and it's
derivatives BITMAPINFOHEADER, BITMAPV4HEADER, BITMAPV5HEADER in the context
of BITMAPINFO and you'll see the ugliness I'm trying to avoid.

Yes, I'm familiar with those structs.

I'm sorry to hear that what I'm doing is broken, because maintaining
duplicate code ala BITMAP*HEADER isn't very appealing (did we really need
four different declarations of the width member with four different names?).

Well, the code will probably work on most compilers even with
inheritence, and if you go the Alloc/Destroy route (internally using
malloc/free or :perator new/delete directly and *not* new/delete
expressions, to avoid invoking destructors implicitly), then any
problems with destructors should go away.

e.g.

/**
** \brief Carries a request and any associated parameters.
**/
struct PNPEXPORT IConcurrentOperations::OpRequest abstract : OpMessage
{
//...
public:
/**
** \brief Frees resources used by this request
**/
void Destroy( void )
{
FreeAgent();
:perator delete(this);
}

protected:
static void* Alloc(std::size_t size)
{
return :perator new(size);
}
};

template <class Derived, class Base>
struct AllocHelper: public Base
{
static Derived* Alloc()
{
Derived* d = static_cast<Derived*>(Base::Alloc(sizeof(Derived)));
d->Init();
//or if you want to stick with constructors:
//new (d) Derived();
return d;
}

//you could add templated overloads with parameters.
};

/**
** \param Base, should be derived from OpNotification or OpRequest
** and provide a default constructor.
**/
template<typename Base>
struct IConcurrentOperations::BufferedMessage
: public AllocHelper<IConcurrentOperations::BufferedMessage<Base>, Base>
{...}

or something like that.

Tom

 

[Ben Voigt]

A strict reading of the PostMessage docs for VC2005 implies it is illegal,

though I don't understand why it should be illegal, since, as you say, you
have control of the pointers at both ends.

Rather the strict reading says that the warnings don't apply to me because
I'm using WM_USER, not "a message in the range below WM_USER".

The point is, you should not be calling constructors if you don't match
the constructor calls with equivalent destructor calls. To avoid
constructor calls, you shouldn't use new-expressions.

If I define operator new myself and have no data members with non-trivial
constructor, then the only extra code being executed are my constructors,
right?

/**
** \param Base, should be derived from OpNotification or OpRequest
** and provide a default constructor.
**/
template<typename Base>
struct IConcurrentOperations::BufferedMessage
: public AllocHelper<IConcurrentOperations::BufferedMessage<Base>, Base>
{...}

That still needs to be explicitly specified for every concrete derived type

or something like that.

Tom

Is this page accurate:
http://www.informit.com/guides/content.asp?g=cplusplus&seqNum=196&rl=1

 

[Tom Widmer [VC++ MVP]]

Rather the strict reading says that the warnings don't apply to me because
I'm using WM_USER, not "a message in the range below WM_USER".

Ahh, I now see that the docs only apply to system messages, apologies
for the confusion.

If I define operator new myself and have no data members with non-trivial
constructor, then the only extra code being executed are my constructors,
right?

Typically, though obviously the compiler can add any extra code it
wants. e.g. in debug builds, the constructors might initialize the
objects memory to 0xDEADBEEF or something before running any other
constructor code, or the object might be registered with a "live
objects" registry.

However, the reason not to use new-expressions is to avoid having to use
delete-expressions, which invoke destructors. If you instead allocate
the memory manually (with malloc, :perator new or even class:perator
new), and then use placement new or an init function to initialize the
object, then the errors you were getting with destructors will not occur
on any compiler.

That still needs to be explicitly specified for every concrete derived type

An alternative would be to stick with the current inheritence heirarchy,
and create a free function (or a member of Base if you prefer):

template <class Derived>
Derived* Alloc()
{
void* p = Base::Alloc(sizeof(Derived));
return new(p) Derived();
}

and then the derived classes just allocate themselves with:

Derived* d = Alloc<Derived>();
rather than with
Derived* d = new Derived();

Again, you can avoid going near the destructors with your Free function,
since you won't use a delete-expression (but rather :perator delete or
free()). You can also declare (but not define) a private operator new
and delete in Base, to make sure no one accidentally does a new or
delete on the derived types.

Is this page accurate:
http://www.informit.com/guides/content.asp?g=cplusplus&seqNum=196&rl=1

Well, it doesn't mention that the location of base objects is
unspecified. It has them directly preceding derived objects, but that
isn't required - they may actually be held in a completely different
part of memory. Only POD objects need use contiguous storage, though in
practice I've not heard of an implementation taking advantage of this
allowance. I suppose, in theory, an implementation could notice that the
base class is constant and can only take a limited set of values,
generate one sub-object for each possible value, and then put a pointer
to the relevent base class sub-object in the derived class on construction.

Tom