EV_WM_COPYDATA - LRESULT or void

Dirk
2012-03-22
2013-07-21
  • Dirk

    Dirk - 2012-03-22

    First of all many thanks to all of you folks working to keep OWL alive! It`s really great to have a continuation up to today!

    We are working on necessary changes to use OWLNext 6.32.2. Our actual code is based on 6.30.8. We noticed changes in "owl/windowev.h" for EV_WM_COPYDATA.

    It used to be:

    #define EV_WM_COPYDATA\  
    {{WM_COPYDATA}, 0, (TAnyDispatcher)::v_HWND_PCOPYDATASTRUCT_Dispatch,\  
    (TMyPMF)NS_OWL::v_HWND_PCOPYDATASTRUCT_Sig<TMyClass>(&TMyClass::EvCopyData)}  
    

    .

    Today in 6.32.2:

    #define EV_WM_COPYDATA\  
    {{WM_COPYDATA}, 0, (::owl::TAnyDispatcher)::owl::LRESULT_HWND_PCOPYDATASTRUCT_Dispatch,\  
    (TMyPMF)::owl::LRESULT_HWND_PCOPYDATASTRUCT_Sig<TMyClass>(&TMyClass::EvCopyData)}  
    

    .

    The macro today should return a LRESULT and formerly it was just void. Looking at "owl/dispatch.h", "dispatch.cpp" and "owl/signature.h" we noticed that all implementations still cover the void case.

    It would be great if someone could decide the one or the other way. Meanwhile we changed the definition of EV_WM_COPYDATA to 'old school'
    v_HWND_PCOPYDATASTRUCT_Sig. I.e. we changed the definition of EV_WM_COPYDATA in "owl/windowev.h" to this:

    #define EV_WM_COPYDATA\  
    {{WM_COPYDATA}, 0, (::owl::TAnyDispatcher)::owl::v_HWND_PCOPYDATASTRUCT_Dispatch,\  
    (TMyPMF)::owl::v_HWND_PCOPYDATASTRUCT_Sig<TMyClass>(&TMyClass::EvCopyData)}  
    

    .

    Again many thanks!

    Regards,
    Dirk

    [Moderator: Fixed Markdown formatting.]

     
    Last edit: Vidar Hasfjord 2013-07-21
  • Ognian Tchernokojev

    As stated in MSDN, WM_COPYDATA returns TRUE if the
    message is processed and FALSE otherwise.

    The 6.30.x implementation of WM_COPYDATA handler always returned true. I think
    it will be better to allow the developers to have control over the return
    value, so I will look at the handlers and correct them

     
  • Vidar Hasfjord

    Vidar Hasfjord - 2012-03-23

    By the way, the message cracker macros for WM_COPYDATA in "windowsx.h" are ambiguous with regard to the signature. The HANDLE... macro treats the return type as void (always returning 0), while the FORWARD... macro treats it as BOOL:

    #define HANDLE_WM_COPYDATA(hwnd, wParam, lParam, fn) \
        ((fn)((hwnd), (HWND)(wParam), (PCOPYDATASTRUCT)lParam), 0L)
    #define FORWARD_WM_COPYDATA(hwnd, hwndFrom, pcds, fn) \
        (BOOL)(UINT)(DWORD)(fn)((hwnd), WM_COPYDATA, (WPARAM)(hwndFrom), (LPARAM)(pcds))
    

    I have based the implementation of the new dispatchers on the message cracking macros in "windowsx.h", but I've found that this file is not quite reliable nor complete. Unfortunately, the file seems to be abandoned by Microsoft. The most recent information I can find about it on MSDN is an article from 1999. For this reason I have had to make my own additions and corrections, and I will make sure to make a correction for HANDLE_WM_COPYDATA as well.

     
    Last edit: Vidar Hasfjord 2013-07-21
  • Vidar Hasfjord

    Vidar Hasfjord - 2012-03-23

    Since the proposed change above will affect the signature anyway, I suggest we
    also change the parameter type PCOPYDATASTRUCT to 'const COPYDATASTRUCT&' in
    accordance with the documentation:

    The receiving application should consider the data read-only. The lParam
    parameter is valid only during the processing of the message. The receiving
    application should not free the memory referenced by lParam. If the receiving
    application must access the data after SendMessage returns, it must copy the
    data into a local buffer.

    For compatibility, we should support the old signature in OWL5_COMPAT mode.
    Altogether this leads to the following dispatch specialization for
    WM_COPYDATA:

    #if OWL_DISPATCH_COMPATIBILITY_
    
    template <> struct TDispatch<WM_COPYDATA> : TDispatchBase<WM_COPYDATA, void>
    {
      template <class T, void (T::*Handler)(HWND sender, PCOPYDATASTRUCT)>
      static TResult Handle(T* i, TParam1 a1, TParam2 a2)
      {return (i->*Handler)(Convert(a1), Convert(a2)), TRUE;}
    
      template <class F>
      static void Forward(F sendMessage, HWND wnd, HWND sender, PCOPYDATASTRUCT p)
      {sendMessage(wnd, MessageId, MakeParam1(sender), MakeParam2(p));}
    };
    
    #else
    
    template <> struct TDispatch<WM_COPYDATA> : TDispatchBase<WM_COPYDATA, bool>
    {
      template <class T, bool (T::*Handler)(HWND sender, const COPYDATASTRUCT&)>
      static TResult Handle(T* i, TParam1 a1, TParam2 a2)
      {
        auto forwarder = [ i ](HWND, HWND hwndSender, PCOPYDATASTRUCT pCopyDataStruct)->bool
          {return (i->*Handler)(hwndSender, *pCopyDataStruct);};
        return HANDLE_WM_COPYDATA(0, a1, a2, forwarder);
      }
    
      template <class F>
      static bool Forward(F sendMessage, HWND wnd, HWND sender, const COPYDATASTRUCT& c)
      {return FORWARD_WM_COPYDATA(wnd, sender, const_cast<PCOPYDATASTRUCT>(&c), sendMessage);}
    };
    
    #endif
    
     
  • Vidar Hasfjord

    Vidar Hasfjord - 2012-04-07

    There's two problems with my new dispatcher above:

    1. A WM_COPYDATA with lParam equal to 0 will crash the application. Although a null-pointer is not a legal argument according to the documentation, this makes the code brittle. Theoretically, a malicious application may send a null-pointer. Hence, a better solution would be to ignore such messages.

    2. The message may be handled by other means than the declared Handle function, which may modify the the contents of the COPYDATASTRUCT pointed to, despite the use of a const-reference in Forward. A solution is to clone the structure in Forward.

    Improved code:

    template <> struct TDispatch<WM_COPYDATA> : TDispatchBase<WM_COPYDATA, bool>
    {
      template <class T, bool (T::*Handler)(HWND sender, const COPYDATASTRUCT&)>
      static TResult Handle(T* i, TParam1 a1, TParam2 a2)
      {
        auto forwarder = [i] (HWND, HWND sender, PCOPYDATASTRUCT p) -> bool
          {return p ? (i->*Handler)(sender, *p) : false;};
        return HANDLE_WM_COPYDATA(0, a1, a2, forwarder);
      }
    
      template <class F>
      static bool Forward(F sendMessage, HWND wnd, HWND sender, const COPYDATASTRUCT& c)
      {
        COPYDATASTRUCT clone = c;
        return FORWARD_WM_COPYDATA(wnd, sender, &clone, sendMessage);
      }
    };
    
     
  • Ognian Tchernokojev

    1. A WM_COPYDATA with lParam equal to 0 will crash the application. Although
      a null-pointer is not a legal argument according to the documentation, this
      makes the code brittle. Theoretically, a malicious application may send a
      null-pointer. Hence, a better solution would be to ignore such messages.

    This is true for other messages as well - a quick search in OWLNext code turns
    out that WM_SIZING and WM_MOVING also have the same vulnerability - sending
    NULL instead of a pointer to a RECT will crash the dispatcher when trying to
    de-reference the parameter.

    Still .. I wonder how much checking we should perform - because a malicious
    application can send a bad pointer instead of a NULL one or other invalid data
    and still cause a crash.

     
  • Vidar Hasfjord

    Vidar Hasfjord - 2012-04-10

    Still .. I wonder how much checking we should perform - because a malicious
    application can send a bad pointer instead of a NULL one or other invalid data
    and still cause a crash.

    True. Checking would seem a bit disproportionate and pointless. I think we may
    ignore the issue for the current code at least.

     
  • Sebastian Ledesma

    We can add the checking code in the debug version (some kind of ASSERT).
    Since a 'maliciuos' application will only crash itself, I don't see mayor
    reason to add additional checks.

    Sebas

     
  • Sebastian Ledesma

    My bad, WM_COPYDATA it's send between applications. So a application can cause
    another to crash if it sends NULL pointer.
    In one application I use WM_COPYDATA to send info to MS-Messenger info on the
    current playing title. I'm not using WM_COPYDATA to receive information, since
    this is the case of interactuation between applications it seems to me
    reasonable to add some checking (even in the release version), but for other
    cases (ie: an application sending a false WM_SIZE with invalid data) it seems
    to me a little too much, we decrease performance for a very rare potential
    attack.

    Sebas

     
  • Vidar Hasfjord

    Vidar Hasfjord - 2012-04-13

    Sebas, having looked a bit closer, I agree that WM_COPYDATA sent between
    applications is a special case that may merit special handling. Since the data
    pointed to is sent across processes, and one process' address space is
    separate from another, I assume Windows does some copying between address
    spaces behind the scene in this case. This should guarantee a valid pointer
    for the receiving process, i.e. that the pointer is dereferencable if not
    null. Although Jogy is right in that the pointer may maliciously point to
    garbage, it is up to the receiving process to validate the data (*), and this
    only depends on the robustness of the handler. Thus there is no way for the
    malicious process to make an successful attack without the presence and
    knowledge of some weakness in the implementation of the handler; provided the
    null pointer case is handled properly.

    (*) Note that the documentation states that "the data being passed must not
    contain pointers or other references to objects not accessible to the
    application receiving the data".

    By the way, in the case of sending a null-pointer, the proposed non-checked
    dispatcher does not crash at the point of converting the null-pointer into a
    reference. The crash only happens on using the reference in the handler. So a
    robust handler can still check for an invalid reference before using it. E.g.:

    bool EvCopyData(HWND sender, const COPYDATASTRUCT& c)
      {
         if (!&c) return false;
         ...
    
     
  • Dirk

    Dirk - 2012-06-01

    Hello to you all!

    Thanks for fixing the problem with the return value bool => void by using v_HWND_PCOPYDATASTRUCT_Sig in OWL5_COMPAT mode.

    There is still a little problem - but it can be fixed quite easily. Could you please use the owl namespace in line 217 of "windowev.h":

    Current line 217 of file "windowev.h":

    {{WM_COPYDATA}, 0, (TAnyDispatcher)::owl::v_HWND_PCOPYDATASTRUCT_Dispatch,
    

    .

    should be:

    {{WM_COPYDATA}, 0, (::owl::TAnyDispatcher)::owl::v_HWND_PCOPYDATASTRUCT_Dispatch,
    

    .

    Thanks again for your great work!

    Regards,
    Dirk

    [Moderator: Fixed Markdown formatting.]

     
    Last edit: Vidar Hasfjord 2013-07-21
  • Vidar Hasfjord

    Vidar Hasfjord - 2012-06-01

    Hi Dirk, thanks for reporting this regression. I've now fixed it; see revision
    1325:

    http://owlnext.svn.sourceforge.net/viewvc/owlnext?view=revision&revision=1325

    This issue can be circumvented by a using-declaration before the response
    table, e.g.:

    using ::owl::TAnyDispatcher;
    DEFINE_RESPONSE_TABLE1(TFoo, TWindow)
    /*...*/
    END_RESPONSE_TABLE;
    
     
  • Anonymous - 2013-07-21

    Hello all, I hope this board is still viewed from time to time.

    I am using C++ Builder XE4.
    OWLNext Version 6.32.5

    I get the following compiler time error...

    [bcc32 Error] s3configframe.cpp(29): E2316 'LRESULT_HWND_PCOPYDATASTRUCT_Dispatch' is not a member of 'owl'

    I have..

    void TConfigMDIFrame::EvCopyData( HWND handle, COPYDATASTRUCT * pCopyDataStruct )
    {
       ...
    } 
    

    .

    Any ideas on how to resolve this compile time error?

    Dan

    [Moderator: Fixed Markdown formatting.]

     
    Last edit: Vidar Hasfjord 2013-07-21
  • Ognian Tchernokojev

    Hi,

    In OWLNext 6.32.3 the signature was changed to return bool instead of void

    Jogy

     


Anonymous

Cancel  Add attachments