Re: private destructor and templates
Ben Voigt wrote:
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.
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.
Incidentally, this is why it's very important to me to avoid virtual
calls to anything. Anyone could drop messages into my queue with an
arbitrary LPARAM.
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.
I see, that seems sensible to avoid.
Ahh, yes. I didn't realise the derived classes have destructors (which is
yet another violation of the requirements on POD data).
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.
That is problematic, since your destructor isn't virtual. You can't
destroy an object through a base class pointer unless it has a virtual
destructor.
If all derived types just add additional POD fields, then I wouldn't need
to destroy it virtually.
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 ::operator new
and ::operator 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?
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?
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.
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.
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 ::operator 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();
::operator delete(this);
}
protected:
static void* Alloc(std::size_t size)
{
return ::operator 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