Issue 1283 - wrong usage of memset, fix sizeof, make proper initializing

As well refactor a couple of places and fix bugs in the affected code
- undeprecate `ATL::CComBSTR`. It's extensively used and only pollutes the compiler output, so we can miss some important warning.
- fix bug in `src/plugin/AdblockPlusClient.cpp`. The size of `tml` (`TOKEN_MANDATORY_LABEL tml = {};`) does not include the length of SID, only the size of structure is expected.
- remove dead code `BOOL CreateLowProcess(WCHAR* wszProcessName, WCHAR* cmdLine)`
- [src/plugin/PluginDomTraverserBase.h] refactor `T* m_cacheElements;` to `std::vector<T> m_cacheElements`
- [src/plugin/PluginDomTraverserBase.h] fix calls where `BSTR` is expected but `wchar_t*` was used
- [src/plugin/PluginDomTraverserBase.h] use scoped lock instead of pairs of `m_criticalSection.Lock();` `m_criticalSection.Unlock();`
- [src/plugin/PluginDomTraverserBase.h] add the check for the value of "abp" attribute. The user of some script could put any arbitrary value there which could cause some security vulnerabilities.
- [src/plugin/PluginDomTraverserBase.h] remove `CPluginDomTraverserBase<T>::ShowNotification(CPluginTab* tab)` because the body was pointless.

[Eric, 2014-09-26 16:36:40]

First of all, this review mixes too many issues to be a good single commit. 

The present comments only address the changes in PluginDomTraverserBase.h. There
are two major changes required here: one to add exception handling to
TraverseDocument and one to rewrite the element cache. A smaller change,
converting the BSTR arguments, depends upon having exception handling and could
have a new argument class. This could be between one and three commits depending
on how you want to organize the changes.

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
File src/plugin/PluginDomTraverserBase.h (right):

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:35: void ShowNotification(CPluginTab*
/*tab*/){}
ShowNotification is completely dead code. The symbol doesn't appear anywhere
else in the code. Even this stub declaration can be deleted.

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:49: typedef
ATL::CComCritSecLock<CComAutoCriticalSection> CriticalSectionLock;
I'm all in favor of using a sentry class instead of manual locking and
unlocking. It's exception safe. Let me be clear about that first.

Let us please, however, use a better naming convention that Microsoft uses. I'd
recommend "CriticalSectionSentry" as a small modification of the name you're
using now.

The existing code uses explicit locking all over the place. If we're going to
use a typedef, we'll want to use it consistently. I would promote this
declaration to one of the headers that's included everywhere.

There are even better ways to deal with locking (namely, as a private base class
that declares the critical section object and provides a semaphore type) but
getting a typedef into the right scope is decent incremental progress.

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:60: std::vector<T> m_cacheElements;
As above, I completely agree with using an STL container for this instead of a
manually allocated array.

The trouble is that we need a multiprocessing-aware cache structure. Right now
it's intermingled with the traverser as a whole, and it's less than clear what
the semaphore is actually protecting. It would be beneficial to move this vector
into its own class, one with its own semaphore, and isolate these issues.

It appears that such a class would only need two methods: an atomic test-and-set
and a reset. On the other hand, it's not clear that that's the right interface
for the cache. See below.

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:114: if (FAILED(pBrowser->get_Busy(&isBusy))
|| isBusy)
Since isBusy is VARIANT_BOOL, it would be more type-appropriate to write 'isBusy
!= VARIANT_FALSE'.

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:148: if
(FAILED(pDoc->getElementsByTagName(ATL::CComBSTR(L"body"), &pBodyCollection)) ||
!pBodyCollection)
The constructor for CComBSTR will throw in low memory situations, and it doesn't
throw a standard C++ library error.

If we have a low-memory situation, it's also likely that extending the cache
(vector::emplace_back) is going to fail and throw bad_alloc. Therefore, if we're
going to add functions that throw here, we also need to add a try-catch
statement around the body of TraverseDocument here.

Either we need catch clauses both for bad_alloc and for whatever CComBSTR
throws, or we need to use our own class here instead of CComBSTR that throws
bad_alloc. If we use two catch clauses, we have to duplicate the code in them.

I don't have a good opinion about the right behavior for the plugin working in a
low memory situation. That's going to take some thought. What I do know is that
the best solution includes some kind of user notification that ABP is not
working as normal because of its situation.

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:294: uint32_t cacheIndex = -1;
Assigning a signed value to an unsigned variable.

Given the code, this variable doesn't really need to be in this scope. It
belongs in the critical section right below. 

The only way that this variable is used later is to computer a pointer to a
cache object. That's the right variable to declare here. (It's also the proper
return value for a test-and-set cache method.)

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:308: return;
This 'return' creates an implicit policy to refuse to cache if the 'abp'
attribute is tampered with. This is a change in behavior from the previous
version. I don't think it's an improvement.

I think a better policy is to treat this case is "not currently cached" and
simply re-enter it. This is analogous to the way the existing code works.

The trouble is that if code is modifying the 'abp' attribute, then we could end
up with a runaway cache vector that grows without bound. As a consequence, we
should have an absolute maximum bound on the vector size, after which it takes
some exceptional action (to be decided, but could be to stop accepting new
elements or could be simply to clear itself and start over).

Also, the fact that web page code is altering 'abp' attribute is evidence of
active tampering. That fact deserves attention. At the very least we should log
this somewhere, even if only to the IE developer console.

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:317: cacheIndex = m_cacheElements.size();
This is the place to put a check on absolute maximum size to detect a runaway
cache.

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:319: m_cacheElements[cacheIndex].Init();
This Init() call is redundant. It could be removed without consequence.

It's a flaw in the original design. It's essentially a second constructor.
Neither the base cache element class in this file nor the subclass in
AdblockPlusDomTraverser.h do anything with Init() other than replicate the
behavior of the constructor. The call to Init() here and both the function
definitions for it can all be removed.

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:345: // Update cache
This cache update will fail if the cache is cleared between the atomic operation
above and this operation. This isn't a flaw in the new code; it was already
present.

In order to be correct, either we need to use a single critical section or we
need a test below to ensure that the index is still valid.

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:362: if (!OnElement(pEl, tag,
&m_cacheElements[cacheIndex], false, indent))
The expression '&m_cacheElements[cacheIndex]' is the one to replace with a cache
element pointer. See comment on the definition of 'cacheIndex' above.

[Eric, 2014-09-26 18:07:23]

I've looked over everything else and found only small issues. If we remove the
changes to PluginDomTraverser.h, the rest of the changes go through as a single
commit.

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
File src/plugin/PluginClass.cpp (left):

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:910: memset(szClassName, 0, MAX_PATH);
The title of the review says "wrong usage of memset". Is this what you meant?
It's not wrong so much as unnecessary.

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:911: className.fill(L'\0');
Are these calls to 'fill' really necessary?

The only case I can think of is when GetClassName writes nothing to the buffer,
but in that case, it would be better to handle the return value as an error and
not ignore it. Or if we don't want to do that, just writing an initial
terminating null would be sufficient. But it's probably better to handle the
error correctly.

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:912: GetClassName(hTabWnd, className.data(),
className.size());
As long as we're making a modification, we might as well change to
'GetClassNameW', which we've been slowly doing elsewhere.

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
File src/plugin/PluginSystem.cpp (right):

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginSystem.cpp:13: std::array<wchar_t, 128> localeLanguage;
The actual buffer size we need, both here for language and below for country, is
9. I didn't know what it actually was until I looked it up.

http://msdn.microsoft.com/en-us/library/windows/desktop/dd373848%28v=vs.85%29...

[Oleksandr, 2014-10-06 20:19:32]

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:424: DWORD trueth = 1;
That typo really jumps at me with this change. Mind changing it to "truth" in
this patch?

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:912: GetClassName(hTabWnd, className.data(),
className.size());
+1
On 2014/09/26 18:07:23, Eric wrote:
> As long as we're making a modification, we might as well change to
> 'GetClassNameW', which we've been slowly doing elsewhere.

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
File src/plugin/PluginDomTraverserBase.h (right):

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:35: void ShowNotification(CPluginTab*
/*tab*/){}
+1
On 2014/09/26 16:36:40, Eric wrote:
> ShowNotification is completely dead code. The symbol doesn't appear anywhere
> else in the code. Even this stub declaration can be deleted.

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:148: if
(FAILED(pDoc->getElementsByTagName(ATL::CComBSTR(L"body"), &pBodyCollection)) ||
!pBodyCollection)
I would be willing to have this committed without the try-catch'es for low
memory cases. I actually think just crashing in that situation is not the worst
thing to do.

On 2014/09/26 16:36:40, Eric wrote:
> The constructor for CComBSTR will throw in low memory situations, and it
doesn't
> throw a standard C++ library error.
> 
> If we have a low-memory situation, it's also likely that extending the cache
> (vector::emplace_back) is going to fail and throw bad_alloc. Therefore, if
we're
> going to add functions that throw here, we also need to add a try-catch
> statement around the body of TraverseDocument here.
> 
> Either we need catch clauses both for bad_alloc and for whatever CComBSTR
> throws, or we need to use our own class here instead of CComBSTR that throws
> bad_alloc. If we use two catch clauses, we have to duplicate the code in them.
> 
> I don't have a good opinion about the right behavior for the plugin working in
a
> low memory situation. That's going to take some thought. What I do know is
that
> the best solution includes some kind of user notification that ABP is not
> working as normal because of its situation.

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:308: return;
I agree. This makes it very easy for someone to disable ABP on a page.
On 2014/09/26 16:36:40, Eric wrote:
> if only to the IE developer console.
Let's not go that far :) Usual debug log would be enough, IMHO.

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:319: m_cacheElements[cacheIndex].Init();
Agreed.
On 2014/09/26 16:36:40, Eric wrote:
> This Init() call is redundant. It could be removed without consequence.
> 
> It's a flaw in the original design. It's essentially a second constructor.
> Neither the base cache element class in this file nor the subclass in
> AdblockPlusDomTraverser.h do anything with Init() other than replicate the
> behavior of the constructor. The call to Init() here and both the function
> definitions for it can all be removed.

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:345: // Update cache
I would prefer the later (ensure that the index is still valid). Otherwise we
would potentially be waiting in OnNavigate, which IE monitors and reports to the
user. We don't want to appear slow for users.

On 2014/09/26 16:36:40, Eric wrote:
> This cache update will fail if the cache is cleared between the atomic
operation
> above and this operation. This isn't a flaw in the new code; it was already
> present.
> 
> In order to be correct, either we need to use a single critical section or we
> need a test below to ensure that the index is still valid.

[sergei, 2014-10-08 15:06:52]

I've split it into several codereview:

Issue 1283 http://codereview.adblockplus.org/5163581322559488/

Issue #1466 http://codereview.adblockplus.org/5698769010032640/

No issue - remove dead code http://codereview.adblockplus.org/5117917230268416/

The changes regarding cache are not extracted, I want to investigate what is
happening there more.

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:912: GetClassName(hTabWnd, className.data(),
className.size());
Despite it's not a real performance trouble, I've fixed it.

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
File src/plugin/PluginDomTraverserBase.h (right):

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:49: typedef
ATL::CComCritSecLock<CComAutoCriticalSection> CriticalSectionLock;
On 2014/09/26 16:36:40, Eric wrote:
> Let us please, however, use a better naming convention that Microsoft uses.
I'd
> recommend "CriticalSectionSentry" as a small modification of the name you're
> using now.

I don't see how "CriticalSectionSentry" is better. I would say it's very common
to use "lock" for the scoped lock. Examples: boost::lock_guard, std::lock_guard,
std:: unique_lock, std::shared_lock, "read write lock"
http://www.boost.org/doc/libs/1_56_0/doc/html/thread/synchronization.html
http://en.cppreference.com/w/cpp/thread/lock_guard
"Sentry" is not used.

> 
> The existing code uses explicit locking all over the place. If we're going to
> use a typedef, we'll want to use it consistently. I would promote this
> declaration to one of the headers that's included everywhere.
>
> There are even better ways to deal with locking (namely, as a private base
class
> that declares the critical section object and provides a semaphore type) but
> getting a typedef into the right scope is decent incremental progress.
It would be convenient to have function like boost::make_lock_guard
(http://www.boost.org/doc/libs/1_56_0/doc/html/thread/synchronization.html#thr...),
which we could use as `auto lock = make_lock_guard(m_mutex);`. But for now I
think it would be enough to have a typedef for the mutex in the class, like
`typedef std::recursive_mutex Mutex;` so we can use any lock as
`std::unique_lock<Mutex> lock(m_mutex);`

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:60: std::vector<T> m_cacheElements;
I completely agree that it's much better to refactor it into a separate cache
class, but the main aim of the current change is to merely fix the bug. As it's
correctly mentioned, currently m_criticalSection is used for the whole class,
and I'm afraid to create even worse bug by changing something here. Let's do it
later, feel free to create an issue.

[Eric, 2014-10-08 16:26:55]

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
File src/plugin/PluginDomTraverserBase.h (right):

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:49: typedef
ATL::CComCritSecLock<CComAutoCriticalSection> CriticalSectionLock;
On 2014/10/08 15:06:52, sergei wrote:
> "Sentry" is not used.

"Sentry" is the word that Stroustrup uses for these kinds of classes. In "The
C++ Programming Language", he talks about them in conjunction with I/O classes.

I have come to dislike the promiscuous use of the word "lock" for almost
everything related to multiprocessing access control. There are lots of things
it can mean, and it's frequently confusing. When there's a more specific word
available, I prefer that.

> But for now I
> think it would be enough to have a typedef for the mutex in the class, like
> `typedef std::recursive_mutex Mutex;` so we can use any lock as
> `std::unique_lock<Mutex> lock(m_mutex);`

I would prefer standard library mutexes over ATL mutexes, for two basic reasons:
(1) it reduces the dependency upon ATL and (2) it uses standard library
exceptions rather than the ATL-specific exceptions. Swapping these classes out
doesn't need to be done as a separate project, but we can do it incrementally
when we touch the code. I've added issue #1467 for this.

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:148: if
(FAILED(pDoc->getElementsByTagName(ATL::CComBSTR(L"body"), &pBodyCollection)) ||
!pBodyCollection)
On 2014/10/06 20:19:32, Oleksandr wrote:
> I would be willing to have this committed without the try-catch'es for low
> memory cases. I actually think just crashing in that situation is not the
worst
> thing to do.

Really? Seems like we can do better than crashing with not much effort.

I'd recommend just disabling the plugin. We've already got user-directed
disabling of the plugin. We can augment that capability with an
internally-generated disabling.

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:345: // Update cache
On 2014/10/06 20:19:32, Oleksandr wrote:
> I would prefer the later (ensure that the index is still valid).

Yes, this would perform better, at the expense of a small amount of extra code.
Seems worth it to me.

[sergei, 2014-10-08 16:58:10]

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
File src/plugin/PluginDomTraverserBase.h (right):

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:148: if
(FAILED(pDoc->getElementsByTagName(ATL::CComBSTR(L"body"), &pBodyCollection)) ||
!pBodyCollection)
I would say that it's better to avoid fiddling around the exceptions here and
implement it as proposed in #1173.

[Eric, 2014-10-08 17:14:40]

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
File src/plugin/PluginDomTraverserBase.h (right):

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:148: if
(FAILED(pDoc->getElementsByTagName(ATL::CComBSTR(L"body"), &pBodyCollection)) ||
!pBodyCollection)
On 2014/10/08 16:58:10, sergei wrote:
> I would say that it's better to avoid fiddling around the exceptions here and
> implement it as proposed in #1173.

#1173 is a bare-minimum of what's necessary. It is neither a final nor ultimate
solution to handling exceptions.

[Oleksandr, 2014-10-08 17:37:46]

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
File src/plugin/PluginDomTraverserBase.h (right):

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:148: if
(FAILED(pDoc->getElementsByTagName(ATL::CComBSTR(L"body"), &pBodyCollection)) ||
!pBodyCollection)
@Eric, I wouldn't say it would be "not much effort". We can, of course add
try-catch to TraverseDocument, but this is not the only place where we use
CComBSTR. Nor is this the only place where problems might appear in low memory
situations, I'm sure. However in all the time the plugin is in use we didn't
really have many complaints about this. We did, however, had a lot of complaints
about other stuff. My point is that the problem is more hypothetical then
veridical, and I think we shouldn't go out of our way to fix it right now. We
should definitely create an P4 issue on this, IMHO.
Shall we vote? Felix?

On 2014/10/08 17:14:40, Eric wrote:
> On 2014/10/08 16:58:10, sergei wrote:
> > I would say that it's better to avoid fiddling around the exceptions here
and
> > implement it as proposed in #1173.
> 
> #1173 is a bare-minimum of what's necessary. It is neither a final nor
ultimate
> solution to handling exceptions.

[Eric, 2014-10-08 18:53:31]

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
File src/plugin/PluginDomTraverserBase.h (right):

http://codereview.adblockplus.org/6237450183639040/diff/5629499534213120/src/...
src/plugin/PluginDomTraverserBase.h:148: if
(FAILED(pDoc->getElementsByTagName(ATL::CComBSTR(L"body"), &pBodyCollection)) ||
!pBodyCollection)
On 2014/10/08 17:37:46, Oleksandr wrote:
> this is not the only place where we use CComBSTR

I doubt I would feel as strongly if I had not already done the work to replace
CComBSTR already and mooted the issue entirely.

> However in all the time the plugin is in use we didn't
> really have many complaints about this. We did, however, had a lot of
complaints
> about other stuff. My point is that the problem is more hypothetical then
> veridical, and I think we shouldn't go out of our way to fix it right now. We
> should definitely create an P4 issue on this, IMHO.
> Shall we vote? Felix?

I am not at all convinced that we've never had reports from users that derive
from exception problems. For example,
https://adblockplus.org/forum/viewtopic.php?f=16&t=24513 contains a report that
looks like it might well be one. You have to dig into what that error might
mean, because it doesn't seem to be documented, but one of the suggested fixes
is to disable third-party extensions.

We have an large bias in our error reporting toward problems that are easy to
replicate and whose causes are understood over those that are hard to replicate
and whose very existence is murky. We have only started to trap exceptions at
all recently and we still have no mechanisms to collect such information from
installations in the field.

In any case, this concern has gotten larger than the present review.