Opened 5 years ago

Closed 3 years ago

#1173 closed change (fixed)

Ensure that C++ exceptions do not propagate out of entry points

Reported by: eric@… Assignee:
Priority: P4 Milestone: Adblock-Plus-for-Internet-Explorer-1.6
Module: Adblock-Plus-for-Internet-Explorer Keywords:
Cc: Blocked By: #2015
Blocking: Platform: Internet Explorer
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

http://codereview.adblockplus.org/6495401389588480/
http://codereview.adblockplus.org/5979857238360064/
https://codereview.adblockplus.org/29332959/

Description (last modified by eric@…)

Background

The plugin code is not friendly to exceptions. In most places in the code, an exception will propagate upward all the way up the stack to the entry point. At the time of creating this ticket, none of the entry points have exception handlers (try-catch block), and so such exceptions will lead to undefined behavior.

Since the C++ idiom is to use exceptions as an ordinary way to signal certain rare circumstances (memory allocation, invalid constructor calls, etc.), the absence of such handlers is a fundamental quality problem.

What to change

  • Ensure that all entry points have an exception handler that will catch all thrown exceptions, that is, a catch(...) statement on the outermost block.
  • Ideally, use entry point calling conventions only for entry points. This will simplify auditing the code to ensure that this change as finished.

Change History (14)

comment:1 Changed 5 years ago by eric@…

  • Description modified (diff)

Entry points (as a rule) don't use C++ calling conventions, and thus tend to be marked by platform-specific annotations, such as WINAPI and STDMETHODIMP, both of whose definitions include the annotation __stdcall.

  • The entry points associated with DLL packaging. They are in Plugin.cpp: DllMain, DllCanUnloadNow, DllRegisterServer, DllUnregisterServer
  • BHO entry points from implementing IObjectWithSite, namely CPluginClass::SetSite.
  • The browser event handler CPluginClass::Invoke.
  • The COM entry points from implementing the user settings class CPluginUserSettings.
  • The event handlers in WBPassthruSink.
  • The COM entry points from implementing IInternetProtocolImpl in the PassthroughAPP library.

I believe this enumerates most of the entry points, though I'm not making any claim that it's all of them.

comment:2 Changed 5 years ago by oleksandr

  • Priority changed from Unknown to P4
  • Ready set

comment:3 Changed 4 years ago by eric@…

  • Review URL(s) modified (diff)

comment:4 Changed 4 years ago by eric@…

Changeset 740 ea510d07ff97f1b50ef5389c0918276d843df391 fixes this for CPluginClass::Invoke

comment:5 follow-up: Changed 4 years ago by sergz

  • Review URL(s) modified (diff)

Wrap more entry points in PluginClass.cpp and add the proposal for the wrapper function. Having these functions we don't have to repeat try-catch and can improve catching for all entry points at once. As well as we don't need to comment each method.

comment:6 Changed 4 years ago by oleksandr

  • Milestone set to Adblock-Plus-for-Internet-Explorer-1.4

comment:7 Changed 4 years ago by oleksandr

  • Blocked By 2015 added

comment:8 Changed 4 years ago by oleksandr

  • Milestone Adblock-Plus-for-Internet-Explorer-1.4 deleted

comment:9 Changed 4 years ago by eric@…

  • Review URL(s) modified (diff)

comment:10 in reply to: ↑ 5 Changed 4 years ago by sergz

  • Review URL(s) modified (diff)

Oleksandr said:

So currently I agree with Eric. Let's just stick to try-catch blocks for now.

comment:11 Changed 3 years ago by eric@…

  • Tester set to Unknown

More entry points.

  • In CPluginClass interface `IOleCommandTarget'
    • QueryStatus()
    • Exec()
  • In CPluginClass, callbacks for UI elements
    • NewStatusProc(), status bar windows
    • PaneWindowProc(), window class of status bar
Last edited 3 years ago by eric@… (previous) (diff)

comment:12 Changed 3 years ago by eric@…

  • Review URL(s) modified (diff)

comment:13 Changed 3 years ago by oleksandr

  • Milestone set to Adblock-Plus-for-Internet-Explorer-Next

comment:14 Changed 3 years ago by oleksandr

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.