Only take into account processes that have our plugin loaded

installer/src/installer-lib/process.h

Index: installer/src/installer-lib/process.h
===================================================================
--- a/installer/src/installer-lib/process.h
+++ b/installer/src/installer-lib/process.h
@@ -2,8 +2,8 @@
  * \file process.h
  */
 
-#ifndef PROCESS_H
-#define PROCESS_H
+#ifndef PROCESS_H
+#define PROCESS_H
 
 #include <vector>
 #include <set>
@@ -80,38 +80,38 @@
 };
 
 //-------------------------------------------------------
-// exe_name_set: case-insensitive wide-string set
+// file_name_set: case-insensitive wide-string set
 //-------------------------------------------------------
 int wcscmpi( const wchar_t * s1, const wchar_t * s2 ) ;

[Wladimir Palant, 2014/03/28 07:30:17]

I realize that this isn't your code but still - why are we dealing with string
pointers and reimplementing basic string functions instead of using
std::wstring?

[Eric, 2014/03/28 12:06:00]

It's mine.
Several interlocking issues:
1) std::wstring does not have a case-insensitive comparison function.
2) Neither does the C library, either in wide strings or otherwise. Look around
the web, there are plenty of functions people have written "stricmp" and
"strcmpi" (both names are common).
3) We are dealing with a data structure not our own, specifically, a pointer to
a Windows API structure with wchar_t[] elements.
4) We are only ever comparing to constant strings that we provide elsewhere.
While it would be easy to initiailize std::wstring with these, they do start out
as wchar_t[].

Therefore, converting the array to a standard string accomplishes little. The
use of this comparison function is isolated to process detection. If it were of
more general applicability, the right C++ way to do this would be to define a
new wide string class with case-insensitive comparison. This can be done simply
by using std::basic_string and deriving a traits class with new comparison
functions. See http://www.cplusplus.com/reference/string/char_traits/compare/
for an example of just that. But even then you still need to write your own
comparison function.

[Oleksandr, 2014/03/28 12:48:35]

That's in TODO:
http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst...


On 2014/03/28 07:30:17, Wladimir Palant wrote:

[Wladimir Palant, 2014/03/28 13:55:24]

Using an existing comparison string is one part of this of course. The more
important part is not using raw pointers however. We use std::wstring wherever
possible to avoid various issues (memory leaks, use-after-free, code that passed
in strings changing or freeing them), this code should not be an exception.

 
-struct exe_name
+struct file_name
 {
   /**
    * Pointer to wide-character string, which is supposed to be null-terminated.
    */
   const wchar_t * name ;
 
-  exe_name( const wchar_t * name ) : name( name ) {} ;
+  file_name( const wchar_t * name ) : name( name ) {} ;
 } ;
 
 template <>
-struct std::less< exe_name >
-  : std::binary_function< exe_name, exe_name, bool >
+struct std::less< file_name >
+  : std::binary_function< file_name, file_name, bool >
 {
-  bool operator()( const exe_name & a, const exe_name & b ) const
+  bool operator()( const file_name & a, const file_name & b ) const
   {
     return wcscmpi( a.name, b.name ) < 0 ; 
   }
 } ;
 
-struct exe_name_set
-  : public std::set< exe_name >
+struct file_name_set
+  : public std::set< file_name >
 {
-  exe_name_set( const wchar_t * exe_name_list[], size_t n_exe_names )
+  file_name_set( const wchar_t * file_name_list[], size_t n_file_names )
   {
-    for ( unsigned int j = 0 ; j < n_exe_names ; ++ j )
+    for ( unsigned int j = 0 ; j < n_file_names ; ++ j )
     {
-      insert( exe_name( exe_name_list[ j ] ) ) ;
+      insert( file_name( file_name_list[ j ] ) ) ;
     }
   }
 } ;
@@ -119,19 +119,21 @@
 //-------------------------------------------------------
 //-------------------------------------------------------
 /**
- * Filter by process name. Comparison is case-insensitive.
+ * Filter by process name. Comparison is case-insensitive. With ABP module loaded
  */
-class process_by_any_exe_name_CI
-  : public std::unary_function< PROCESSENTRY32W, bool >
+class process_by_any_exe_with_any_module
+  : public std::binary_function< PROCESSENTRY32W, file_name_set, bool >
 {
-  const exe_name_set & names ;
+  const file_name_set & processNames ;
+  const file_name_set & moduleNames;
 public:
   bool operator()( const PROCESSENTRY32W & ) ;
-  process_by_any_exe_name_CI( const exe_name_set & names )
-    : names( names )
+  process_by_any_exe_with_any_module( const file_name_set & names, const file_name_set & moduleNames )
+    : processNames( names ), moduleNames( moduleNames )
   {}
 } ;
 
+
 //-------------------------------------------------------
 // Process utility functions.
 //-------------------------------------------------------
@@ -283,6 +285,64 @@
   typedef PROCESSENTRY32W * Pointer ;
 } ;
 
+class ModulesSnapshot
+{
+  /**
+   * Handle to the process snapshot.
+   */
+  Windows_Handle handle ;

[Wladimir Palant, 2014/03/28 07:30:17]

Style nit: no space before the semicolon please (and eventually we need to fix
it elsewhere in that codebase as well).

+
+  /**
+   * Buffer for reading a single module entry out of the snapshot.
+   */
+  MODULEENTRY32W module;
+
+  /**
+   * Copy constructor declared private and not defined.
+   *
+   * \par Implementation
+   *   Add "= delete" for C++11.
+   */
+  ModulesSnapshot( const ModulesSnapshot & ) ;
+
+  /**
+   * Copy assignment declared private and not defined.
+   *
+   * \par Implementation
+   *   Add "= delete" for C++11.
+   */
+  ModulesSnapshot operator=( const ModulesSnapshot & ) ;
+
+
+public:
+  /**
+   * Default constructor takes the snapshot.
+   */
+  ModulesSnapshot(DWORD processId) ;
+
+  /**
+   * Return a pointer to the first process in the snapshot.
+   */
+  MODULEENTRY32W * begin() ;
+
+  /**
+   * The end pointer is an alias for the null pointer.
+   */
+  inline MODULEENTRY32W * end() const { return 0 ; }
+
+  /**
+   * Return a pointer to the next process in the snapshot.
+   * begin() must have been called first.
+   */
+  MODULEENTRY32W * next() ;
+
+  /**
+   * Type definition for pointer to underlying structure.
+   */
+  typedef MODULEENTRY32W * Pointer ;

[Wladimir Palant, 2014/03/28 07:30:17]

What is that type good for? Faking a generic iteration interface?

[Oleksandr, 2014/03/28 12:48:35]

Yes.
On 2014/03/28 07:30:17, Wladimir Palant wrote:

+} ;
+
+
 //-------------------------------------------------------
 // initialize_process_list
 //-------------------------------------------------------
@@ -465,12 +525,15 @@
    * The argument of the filter constructor is a set by reference.
    * Since it does not make a copy for itself, we define it as a class member to provide its allocation.
    */
-  exe_name_set exe_names ;
+  file_name_set file_names ;
+  file_name_set module_names ;

[Wladimir Palant, 2014/03/28 07:30:17]

Nit: The comment no longer applies here, I guess module_names needs its own
comment.

[Eric, 2014/03/28 12:06:00]

Yes. The comment beginning /** is a Doxygen comment.

 
   /**
    * Filter function object matches on any of the exe names specified in the constructor.
    */
-  process_by_any_exe_name_CI filter ;
+//  process_by_any_file_name_CI filter ;

[Wladimir Palant, 2014/03/28 07:30:17]

Please remove this line instead of commenting it.

+
+  process_by_any_exe_with_any_module filter ;
 
   /**
    * Copy function object copies just the process ID.
@@ -521,8 +584,8 @@
   } ;
 
 public:
-  Process_Closer( Snapshot & snapshot, const wchar_t * exe_name_list[], size_t n_exe_names )
-    : snapshot( snapshot ), exe_names( exe_name_list, n_exe_names ), filter( exe_names )
+  Process_Closer( Snapshot & snapshot, const wchar_t * file_name_list[], size_t n_file_names, const wchar_t * module_name_list[], size_t n_module_names )
+    : snapshot( snapshot ), file_names( file_name_list, n_file_names ), module_names(module_name_list, n_module_names), filter( file_names, module_names )

[Wladimir Palant, 2014/03/28 07:30:17]

With this signature getting very complex, it is probably a good idea to have a
templated version, similarly to how sprintf_s() does it:

template <size_t n_file_names, size_t n_module_names>
Process_Closer(Snapshot& snapshot, const wchar_t*
(&file_name_list)[n_file_names], const wchar_t*
(&module_name_list)[n_module_names])

[Oleksandr, 2014/03/28 12:48:35]

This won't work since we need to be able to pass arrays with size 0. Also, I
don't think it would increase readability much. 
On 2014/03/28 07:30:17, Wladimir Palant wrote:

[Eric, 2014/03/28 13:15:01]

It should work for that. Zero is a valid template argument. As long as an empty
array is explicitly initialized, either with an empty list or a zero length, the
compiler knows the array length.
Its main advantage is to allow elimination of explicit length arguments in the
call, at the slight expense some complexity in the declaration. The length
argument must match with the array argument. Since the template arguments would
be derived, it eliminates a source of error.

[Oleksandr, 2014/03/28 13:49:27]

Hm. Care to elaborate on this? The C++ specs seems to suggest that it isn't
possible to initialize an array with zero length ($6.7.5.2/1)

"
In addition to optional type qualifiers and the keyword static, the [ and ] may
delimit
an expression or *. If they delimit an expression (which specifies the size of an
array), the
expression shall have an integer type. If the expression is a constant
expression, it shall
have a value greater than zero"

[Eric, 2014/03/28 15:13:19]

Oops. I was wrong about that; you're right. The tiny nits one never needs to
know about ...

On the other hand, in order to support zero-length arrays, the better way to do
it is with another constructor that omits the argument entirely. Given that
semantics also differ (as discussed elsewhere), it's probably the right thing to
do.

   {
     update() ;
   }