Only take into account processes that have our plugin loaded

installer/src/installer-lib/process.h

 /**
  * \file process.h
  */
 
 #ifndef PROCESS_H
 #define PROCESS_H
 
 #include <vector>
 #include <set>
 #include <algorithm>
@@ skipping 64 matching lines @@
    * \par Implementation
    *   Add "= delete" for C++11.
    */
   Windows_Handle operator=( const Windows_Handle & ) ;
 
 };
 
 //-------------------------------------------------------
 // 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 file_name
 {
   /**
    * Pointer to wide-character string, which is supposed to be null-terminated.
    */
   const wchar_t * name ;
 
   file_name( const wchar_t * name ) : name( name ) {} ;
 } ;
 
 template <>
 struct std::less< file_name >
   : std::binary_function< file_name, file_name, bool >
 {
   bool operator()( const file_name & a, const file_name & b ) const
   {
     return wcscmpi( a.name, b.name ) < 0 ; 
   }
 } ;
 
 struct file_name_set
   : public std::set< file_name >
 {
+  file_name_set(){};
+
   file_name_set( const wchar_t * file_name_list[], size_t n_file_names )
   {
     for ( unsigned int j = 0 ; j < n_file_names ; ++ j )
     {
       insert( file_name( file_name_list[ j ] ) ) ;
     }
   }
 } ;
 
 //-------------------------------------------------------
@@ skipping 139 matching lines @@
   Snapshot() ;
 
   /**
    * Reconstruct the current instance with a new system snapshot.
    */
   void refresh() ;
 
   /**
    * Return a pointer to the first process in the snapshot.
    */
-  PROCESSENTRY32W * begin() ;
-
-  /**
-   * The end pointer is an alias for the null pointer.
-   */
-  inline PROCESSENTRY32W * end() const { return 0 ; }
+  PROCESSENTRY32W * first() ;
 
   /**
    * Return a pointer to the next process in the snapshot.
    * begin() must have been called first.
    */
   PROCESSENTRY32W * next() ;
-
-  /**
-   * Type definition for pointer to underlying structure.
-   */
-  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).

+  Windows_Handle handle;
 
   /**
    * 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 & ) ;
+  ModulesSnapshot(const ModulesSnapshot&);
 
   /**
    * Copy assignment declared private and not defined.
    *
    * \par Implementation
    *   Add "= delete" for C++11.
    */
-  ModulesSnapshot operator=( const ModulesSnapshot & ) ;
+  ModulesSnapshot operator=(const ModulesSnapshot&);
 
 
 public:
   /**
    * Default constructor takes the snapshot.
    */
-  ModulesSnapshot(DWORD processId) ;
+  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 ; }
+  MODULEENTRY32W* first();
 
   /**
    * 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:

-} ;
+  MODULEENTRY32W* next();
+};
 
 
 //-------------------------------------------------------
 // initialize_process_list
 //-------------------------------------------------------
 /**
  * \tparam T The type into which a PROCESSENTRY32W struture is extracted.
  * \tparam Admittance Function type for argument 'admit'
  * \tparam Extractor Function type for argument 'extract'
  * \param admit A unary predicate function class that determines what's included
  *      A process appears in the list only if the predicate returns true. 
  *      The use of this predicate is analogous to that in std::copy_if.
  * \param convert A conversion function that takes a PROCESSENTRY32W as input argument and returns an element of type T.
  */
-template< class T, class Admittance, class Extractor >
-void initialize_process_list( std::vector< T > & v, Snapshot & snap, Admittance admit = Admittance(), Extractor extract = Extractor() )
-{
-  Snapshot::Pointer p = snap.begin() ;
-  while ( p != snap.end() )
-  {
-    if ( admit( * p ) )
+template<class T, class Admittance, class Extractor>
+void initialize_process_list(std::vector<T>& v, Snapshot& snap, Admittance admit = Admittance(), Extractor extract = Extractor())
+{
+  PROCESSENTRY32W* p = snap.first();
+  while (p != NULL)
+  {
+    if (admit(*p ))
     {
       /*
         * We don't have C++11 emplace_back, which can construct the element in place.
         * Instead, we copy the return value of the converter.
         */
-      v.push_back( extract( * p ) );
-    }
-    p = snap.next() ;
-  }
-} ;
+      v.push_back(extract(*p));
+    }
+    p = snap.next();
+  }
+};
 
 //-------------------------------------------------------
 // initialize_process_set
 //-------------------------------------------------------
 /**
  * \tparam T The type into which a PROCESSENTRY32W struture is extracted.
  * \tparam Admittance Function type for argument 'admit'
  * \tparam Extractor Function type for argument 'extract'
  * \param admit A unary predicate function class that determines what's included
  *      A process appears in the list only if the predicate returns true. 
  *      The use of this predicate is analogous to that in std::copy_if.
  * \param convert A conversion function that takes a PROCESSENTRY32W as input argument and returns an element of type T.
  */
-template< class T, class Admittance, class Extractor >
-void initialize_process_set( std::set< T > & set, Snapshot & snap, Admittance admit = Admittance(), Extractor extract = Extractor() )
-{
-  Snapshot::Pointer p = snap.begin() ;
-  while ( p != snap.end() )
-  {
-    if ( admit( * p ) )
-    {
-      set.insert( extract( * p ) );
-    }
-    p = snap.next() ;
-  }
-} ;
+template<class T, class Admittance, class Extractor>
+void initialize_process_set(std::set< T > & set, Snapshot& snap, Admittance admit = Admittance(), Extractor extract = Extractor())
+{
+  PROCESSENTRY32W* p = snap.first();
+  while (p != NULL)
+  {
+    if (admit(*p))
+    {
+      set.insert(extract(*p));
+    }
+    p = snap.next();
+  }
+};
 
 //-------------------------------------------------------
 // enumerate_windows
 //-------------------------------------------------------
 
 /**
  * States of a window enumeration.
  */
 typedef enum
 {
@@ skipping 108 matching lines @@
    */
   std::set< DWORD > pid_set ;
 
   /**
    * Set of executable names by which to filter.
    *
    * 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.
    */
   file_name_set file_names ;
+  /**
+   * Set of module (DLL) names by which to filter.
+   */
   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_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.
    */
   copy_PID copy ;
 
   /** 
    * Snapshot of running processes.
@@ skipping 32 matching lines @@
       if ( ! b )
       {
         // Assert the process that created the window is not in our pid_set
         return true ;
       }
       return f( window ) ;
     }
   } ;
 
 public:
-  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.

+  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])
+    : snapshot(snapshot), file_names(file_name_list, n_file_names), module_names(module_name_list, n_module_names), filter(file_names, module_names)
+  {
+    update() ;
+  }
+  template <size_t n_file_names>
+  Process_Closer(Snapshot & snapshot, const wchar_t * (&file_name_list)[n_file_names])
+    : snapshot(snapshot), file_names(file_name_list, n_file_names), module_names(), filter(file_names, module_names)
   {
     update() ;
   }
 
   /**
    * Refresh our state to match the snapshot state.
    */
   void refresh()
   {
     pid_set.clear() ;
@@ skipping 12 matching lines @@
   }
 
   /*
    * Shut down every process in the pid_set.
    */
   bool shut_down() ;
 
 } ;
 
 #endif