Issue 6003395731128320: Only take into account processes that have our plugin loaded (Closed)
|
Created:
March 27, 2014, 3:29 p.m. by Oleksandr Modified:
April 1, 2014, 10:52 a.m. Visibility:
Public. |
DescriptionOnly take into account processes that have our plugin loaded
Patch Set 1 #
Total comments: 18
Patch Set 2 : Changes for x64 custom action and addressing comments #
Total comments: 24
Patch Set 3 : Addressing comments #Patch Set 4 : Do not fake C++ iterators #Patch Set 5 : Simplify Process_Closer constructor #
MessagesTotal messages: 17
Only cosmetics (names, really), a bit of dead code, and one what seems to be spurious "return". Everything else looks fine. http://codereview.adblockplus.org/6003395731128320/diff/5629499534213120/inst... File installer/src/installer-lib/process.cpp (right): http://codereview.adblockplus.org/6003395731128320/diff/5629499534213120/inst... installer/src/installer-lib/process.cpp:35: sprintf(tmp, "Invalid handle. Last error: %d", GetLastError()); Using sprintf() seems safe here, but it's such a buffer-leaking hazard generally that it's better to omit it entirely. Not relevant here, though. This code really deserves use of a proper exception class that packages Windows API errors consistently. Please just mark this in a comment as TODO and I'll take care or it. http://codereview.adblockplus.org/6003395731128320/diff/5629499534213120/inst... installer/src/installer-lib/process.cpp:65: bool moduleFound = false; We can eliminate 'moduleFound' http://codereview.adblockplus.org/6003395731128320/diff/5629499534213120/inst... installer/src/installer-lib/process.cpp:69: return true; We should document that if no module names are present, that the filter uses only one criterion and not two. Ordinarily, if there were a requirement that there be some element in a set and that set were empty, that the requirement couldn't be satisfied. That would mean "return false" here. Documenting that an empty set means "don't filter on this set" would put that aright. http://codereview.adblockplus.org/6003395731128320/diff/5629499534213120/inst... installer/src/installer-lib/process.cpp:72: return true; Stub return for testing something, I assume? http://codereview.adblockplus.org/6003395731128320/diff/5629499534213120/inst... installer/src/installer-lib/process.cpp:78: moduleFound = true; return true http://codereview.adblockplus.org/6003395731128320/diff/5629499534213120/inst... installer/src/installer-lib/process.cpp:84: return moduleFound; return false http://codereview.adblockplus.org/6003395731128320/diff/5629499534213120/inst... installer/src/installer-lib/process.cpp:212: void ModulesSnapshot::refresh(DWORD processId) refresh() not needed. http://codereview.adblockplus.org/6003395731128320/diff/5629499534213120/inst... installer/src/installer-lib/process.cpp:214: handle = ::CreateToolhelp32Snapshot( TH32CS_SNAPMODULE | TH32CS_SNAPMODULE32, processId ) ; Not that it matters, but processID isn't initialized in the constructor. http://codereview.adblockplus.org/6003395731128320/diff/5629499534213120/inst... File installer/src/installer-lib/process.h (right): http://codereview.adblockplus.org/6003395731128320/diff/5629499534213120/inst... installer/src/installer-lib/process.h:140: class process_by_any_exe_name_CI_w_ABP This is not ABP-specific until after we pass it ABP module names. I would have called it something like process_by_any_exe_with_any_module_CI. In retrospect, I'd drop the _CI suffix, since process and module names are case-insensitive strings, and the CI comparison is part of their type. http://codereview.adblockplus.org/6003395731128320/diff/5629499534213120/inst... installer/src/installer-lib/process.h:143: const exe_name_set & names ; This should be renamed to process_names (or the like) since we have two different names to distinguish. http://codereview.adblockplus.org/6003395731128320/diff/5629499534213120/inst... installer/src/installer-lib/process.h:303: We need some documentation here about CreateToolhelp32Snapshot behaves. We need a second kind of snapshot because that function will snap "all process for the system" at once, but only "all modules for a process" and not "all modules for the system". http://codereview.adblockplus.org/6003395731128320/diff/5629499534213120/inst... installer/src/installer-lib/process.h:319: DWORD processId; Only needed for refresh(), which we don't need. http://codereview.adblockplus.org/6003395731128320/diff/5629499534213120/inst... installer/src/installer-lib/process.h:347: void refresh(DWORD processId) ; The way this is being used, I don't believe we need a refresh method. This class is only being instantiated in the process filter, and that instance is transient. http://codereview.adblockplus.org/6003395731128320/diff/5629499534213120/inst... installer/src/installer-lib/process.h:555: exe_name_set module_names ; I'd use dll_names. We configure these with .EXE and .DLL file names. The Windows terms are process and module. I used the file names in this class as a slightly-easier documentation for the user about what to expect. Also, we should likely rename 'exe_name_set', since it's now holding more that .EXE names. Probably 'file_name_set' http://codereview.adblockplus.org/6003395731128320/diff/5629499534213120/inst... installer/src/installer-lib/process.h:613: Process_Closer( Snapshot & snapshot, const wchar_t * exe_name_list[], size_t n_exe_names, const wchar_t * module_name_list[], size_t n_module_names ) Likewise dll_name_list etc. here.
http://codereview.adblockplus.org/6003395731128320/diff/5629499534213120/inst... File installer/src/installer-lib/process.h (right): http://codereview.adblockplus.org/6003395731128320/diff/5629499534213120/inst... installer/src/installer-lib/process.h:124: class process_by_any_exe_name_CI Since this filter isn't being used any more, it can be moved into the unit test code. I wouldn't delete it entirely, because it's useful to have the simpler code around (with tests) already working in case we might derive something else from it.
http://codereview.adblockplus.org/6003395731128320/diff/5629499534213120/inst... File installer/src/installer-lib/process.h (right): http://codereview.adblockplus.org/6003395731128320/diff/5629499534213120/inst... installer/src/installer-lib/process.h:555: exe_name_set module_names ; I actually think module_names is more consistent, since it's called modules in Windows functions. On 2014/03/27 16:52:44, Eric wrote: > I'd use dll_names. We configure these with .EXE and .DLL file names. The Windows > terms are process and module. I used the file names in this class as a > slightly-easier documentation for the user about what to expect. > > Also, we should likely rename 'exe_name_set', since it's now holding more that > .EXE names. Probably 'file_name_set'
http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... File installer/src/installer-lib/process.cpp (right): http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... installer/src/installer-lib/process.cpp:37: sprintf(tmp, "Invalid handle. Last error: %d", GetLastError()); sprintf() is deprecated, please use sprintf_s() instead. Also a nit: Please use spaces for indentation. http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... installer/src/installer-lib/process.cpp:57: //------------------------------------------------------- I guess I am missing the point of having of this comment - what value does it add? http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... installer/src/installer-lib/process.cpp:74: } Nit: Please use spaces for indentation. http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... installer/src/installer-lib/process.cpp:189: : handle( ::CreateToolhelp32Snapshot( TH32CS_SNAPMODULE | TH32CS_SNAPMODULE32, processId ) ) Style nit: That's rather unusual style in this file - spaces after opening parenthesis and before closing parenthesis, usually we don't do that in our code. Please remove them in your changes, it needs to be fixed for the rest of the file as well eventually. http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... installer/src/installer-lib/process.cpp:202: } This fakes a C++ iterator interface but does so incompletely - the iterator should be a reference, next() and end() have to be methods of the iterator. IMHO in that case it is better to not even pretend being an iterator. These methods can be called first() and next() and the caller can simply check whether it gets NULL. http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... File installer/src/installer-lib/process.h (right): http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... installer/src/installer-lib/process.h:85: int wcscmpi( const wchar_t * s1, const wchar_t * s2 ) ; 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? http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... installer/src/installer-lib/process.h:293: Windows_Handle handle ; Style nit: no space before the semicolon please (and eventually we need to fix it elsewhere in that codebase as well). http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... installer/src/installer-lib/process.h:342: typedef MODULEENTRY32W * Pointer ; What is that type good for? Faking a generic iteration interface? http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... installer/src/installer-lib/process.h:529: file_name_set module_names ; Nit: The comment no longer applies here, I guess module_names needs its own comment. http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... installer/src/installer-lib/process.h:534: // process_by_any_file_name_CI filter ; Please remove this line instead of commenting it. http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... installer/src/installer-lib/process.h:588: : snapshot( snapshot ), file_names( file_name_list, n_file_names ), module_names(module_name_list, n_module_names), filter( file_names, module_names ) 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])
http://codereview.adblockplus.org/6003395731128320/diff/5629499534213120/inst... File installer/src/installer-lib/process.h (right): http://codereview.adblockplus.org/6003395731128320/diff/5629499534213120/inst... installer/src/installer-lib/process.h:124: class process_by_any_exe_name_CI On 2014/03/27 16:56:51, Eric wrote: > I wouldn't delete it entirely, because it's useful to have the simpler > code around (with tests) already working in case we might derive something else > from it. I strongly prefer to remove any unused code that's around. It isn't useful but rather a maintenance burden. Should we ever need that code (which is very uncertain) we know where to find it - in the repository history.
On 2014/03/28 07:34:55, Wladimir Palant wrote: > I strongly prefer to remove any unused code that's around. > [...] Should we ever need that code (which is very > uncertain) The class in question is still used in the unit tests, and not just to test itself. It's a filter object that's used in conjunction with the file name sets used in the process closer, and there are tests for those classes that use the earlier filter class. The second reason I stated, deriving new code from it, is a secondary concern.
http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... File installer/src/installer-lib/process.cpp (right): http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... installer/src/installer-lib/process.cpp:202: } On 2014/03/28 07:30:17, Wladimir Palant wrote: > This fakes a C++ iterator interface but does so incompletely Yep. It really ought be a proper iterator class, but that can't be done. It's a wrapper around a Windows system call with state implicitly embedded in a handle. There's another one just like it the Snapshot class, which uses the same system call to retrieve all processes in the system; this one retrieves all modules for a process. > the iterator > should be a reference, next() and end() have to be methods of the iterator. I would have done exactly that had it been possible. The very short explanation: iterators must be copyable. > in that case it is better to not even pretend being an iterator. These methods > can be called first() and next() and the caller can simply check whether it gets > NULL. Using the iterator naming convention makes the loops like just like other ones in C++. It's more indicative when reading the code what's going on, assuming one is used to reading C++ code. http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... File installer/src/installer-lib/process.h (right): http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... installer/src/installer-lib/process.h:85: int wcscmpi( const wchar_t * s1, const wchar_t * s2 ) ; On 2014/03/28 07:30:17, Wladimir Palant wrote: > I realize that this isn't your code It's mine. > why are we dealing with string > pointers and reimplementing basic string functions instead of using > std::wstring? 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. http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... installer/src/installer-lib/process.h:529: file_name_set module_names ; On 2014/03/28 07:30:17, Wladimir Palant wrote: > module_names needs its own comment. Yes. The comment beginning /** is a Doxygen comment.
Also. LGTM at this point. The one remaining issue (sprintf) we'll deal with in a different way.
http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... File installer/src/installer-lib/process.cpp (right): http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... installer/src/installer-lib/process.cpp:202: } I actually like the way it is implemented. The iterator in this case is "handle" and begin(), next() and end() are all there and can be considered methods of the "handle". I would like to leave it as is. Can you please give an example of why this isn't a good approach? On 2014/03/28 07:30:17, Wladimir Palant wrote: > This fakes a C++ iterator interface but does so incompletely - the iterator > should be a reference, next() and end() have to be methods of the iterator. IMHO > in that case it is better to not even pretend being an iterator. These methods > can be called first() and next() and the caller can simply check whether it gets > NULL. http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... File installer/src/installer-lib/process.h (right): http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... installer/src/installer-lib/process.h:85: int wcscmpi( const wchar_t * s1, const wchar_t * s2 ) ; That's in TODO: http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst... On 2014/03/28 07:30:17, Wladimir Palant wrote: > 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? http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... installer/src/installer-lib/process.h:342: typedef MODULEENTRY32W * Pointer ; Yes. On 2014/03/28 07:30:17, Wladimir Palant wrote: > What is that type good for? Faking a generic iteration interface? http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... installer/src/installer-lib/process.h:588: : snapshot( snapshot ), file_names( file_name_list, n_file_names ), module_names(module_name_list, n_module_names), filter( file_names, module_names ) 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: > 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])
http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... File installer/src/installer-lib/process.h (right): http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... installer/src/installer-lib/process.h:588: : snapshot( snapshot ), file_names( file_name_list, n_file_names ), module_names(module_name_list, n_module_names), filter( file_names, module_names ) On 2014/03/28 12:48:35, Oleksandr wrote: > This won't work since we need to be able to pass arrays with size 0. 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. > Also, I don't think it would increase readability much. 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.
http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... File installer/src/installer-lib/process.h (right): http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... installer/src/installer-lib/process.h:588: : snapshot( snapshot ), file_names( file_name_list, n_file_names ), module_names(module_name_list, n_module_names), filter( file_names, module_names ) On 2014/03/28 13:15:01, Eric wrote: > On 2014/03/28 12:48:35, Oleksandr wrote: > > This won't work since we need to be able to pass arrays with size 0. > > 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. > 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"
http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... File installer/src/installer-lib/process.cpp (right): http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... installer/src/installer-lib/process.cpp:202: } On 2014/03/28 12:48:35, Oleksandr wrote: > Can you please give an example of why > this isn't a good approach? This is simply misleading, one looks at the loop and expects a real iterator. The difference is only obvious at the second glance, this invites overlooking issues or making mistakes when using this interface in own code. http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... File installer/src/installer-lib/process.h (right): http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... installer/src/installer-lib/process.h:85: int wcscmpi( const wchar_t * s1, const wchar_t * s2 ) ; On 2014/03/28 12:48:35, Oleksandr wrote: > That's in TODO: > http://codereview.adblockplus.org/5675960980471808/diff/5629499534213120/inst... 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.
http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... File installer/src/installer-lib/process.cpp (right): http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... installer/src/installer-lib/process.cpp:202: } Fixed in Patchset 4 On 2014/03/28 13:55:24, Wladimir Palant wrote: > On 2014/03/28 12:48:35, Oleksandr wrote: > > Can you please give an example of why > > this isn't a good approach? > > This is simply misleading, one looks at the loop and expects a real iterator. > The difference is only obvious at the second glance, this invites overlooking > issues or making mistakes when using this interface in own code.
http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... File installer/src/installer-lib/process.h (right): http://codereview.adblockplus.org/6003395731128320/diff/5676830073815040/inst... installer/src/installer-lib/process.h:588: : snapshot( snapshot ), file_names( file_name_list, n_file_names ), module_names(module_name_list, n_module_names), filter( file_names, module_names ) On 2014/03/28 13:49:27, Oleksandr wrote: > The C++ specs seems to suggest that it isn't > possible to initialize an array with zero length 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.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
