Issue 11756012: Enhanced Protected Mode support (Closed)
DescriptionI need to do some more testing (specifically on XP and Windows 7), but this should be ok.
Patch Set 1 #
Total comments: 31
Patch Set 2 : Addressing comments #
Total comments: 11
MessagesTotal messages: 27
I think I forgot to send out email about review, so here it is.
http://codereview.adblockplus.org/11756012/diff/1/adblockplus.gyp File adblockplus.gyp (right): http://codereview.adblockplus.org/11756012/diff/1/adblockplus.gyp#newcode138 adblockplus.gyp:138: '$(WINDDKDIR)/lib/ATL/amd64', Nit: probably makes sense to have the order of entries the same for ia32 and x64, meaning WINDDKDIR at the last entry. http://codereview.adblockplus.org/11756012/diff/1/src/engine/Main.cpp File src/engine/Main.cpp (right): http://codereview.adblockplus.org/11756012/diff/1/src/engine/Main.cpp#newcode407 src/engine/Main.cpp:407: Communication::browserSID = argv[2]; This will cause a crash if too few parameters are passed in, please check the number of parameters as done above. http://codereview.adblockplus.org/11756012/diff/1/src/plugin/AdblockPlusClien... File src/plugin/AdblockPlusClient.cpp (right): http://codereview.adblockplus.org/11756012/diff/1/src/plugin/AdblockPlusClien... src/plugin/AdblockPlusClient.cpp:32: DWORD dwLength = 0; Nit: Since when are we using Hungarian notation? I'd suggest dropping the prefix in the variable names. http://codereview.adblockplus.org/11756012/diff/1/src/plugin/AdblockPlusClien... src/plugin/AdblockPlusClient.cpp:35: if (!GetTokenInformation(token, TokenAppContainerSid, (LPVOID) acSid, 0, &dwLength) && GetLastError() == ERROR_INSUFFICIENT_BUFFER) Nit: I think that the explicit cast to LPVOID here and below is unnecessary. http://codereview.adblockplus.org/11756012/diff/1/src/plugin/AdblockPlusClien... src/plugin/AdblockPlusClient.cpp:37: acSid = (TOKEN_APPCONTAINER_INFORMATION*)HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, dwLength); Why are we using HeapAlloc() rather than "new" here? The latter can be used with a smart pointer which allows ignoring the cleanup. Speaking of cleanup, are we leaking that pointer? Nit: use static_cast for the type conversion here? http://codereview.adblockplus.org/11756012/diff/1/src/plugin/AdblockPlusClien... src/plugin/AdblockPlusClient.cpp:46: if ((acSid != NULL) && (acSid->TokenAppContainer != NULL)) Nit: the extra parentheses are unnecessary. http://codereview.adblockplus.org/11756012/diff/1/src/plugin/AdblockPlusClien... src/plugin/AdblockPlusClient.cpp:101: catch(...) I don't really like seeing "catch all", what kind of error are you expecting here that we react to by spawing the engine? http://codereview.adblockplus.org/11756012/diff/1/src/shared/Communication.cpp File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/11756012/diff/1/src/shared/Communication.cp... src/shared/Communication.cpp:8: #include <strsafe.h> Please move that up, to the built-in include files. http://codereview.adblockplus.org/11756012/diff/1/src/shared/Communication.cp... src/shared/Communication.cpp:204: Communication::Pipe::Pipe(const std::wstring& pipeName, Communication::Pipe::Mode mode) Nit: Please keep an empty line before this one. http://codereview.adblockplus.org/11756012/diff/1/src/shared/Communication.cp... src/shared/Communication.cpp:232: if (GetLogonSid(hToken, &pLogonSid) && CreateObjectSecurityDescriptor(pLogonSid, &securitydescriptor) ) Why do we need to allow the logon sid? Is it for testing only or can the plugin run both inside and outside the app container? http://codereview.adblockplus.org/11756012/diff/1/src/shared/Communication.cp... src/shared/Communication.cpp:235: sa.bInheritHandle = TRUE; I think that we still want to set this in any case. In fact, it's probably a good idea to alwaysset sa.nLength as well. http://codereview.adblockplus.org/11756012/diff/1/src/shared/Communication.cp... src/shared/Communication.cpp:239: pipe = CreateNamedPipe(pipeName.c_str(), PIPE_ACCESS_DUPLEX, PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT, Please use CreateNamedPipeW explicitly, as we had it before. http://codereview.adblockplus.org/11756012/diff/1/src/shared/Communication.cp... src/shared/Communication.cpp:241: } Bad indentation? http://codereview.adblockplus.org/11756012/diff/1/src/shared/Communication.h File src/shared/Communication.h (right): http://codereview.adblockplus.org/11756012/diff/1/src/shared/Communication.h#... src/shared/Communication.h:194: PSECURITY_DESCRIPTOR securitydescriptor; Why do we need this as a local variable? It is only used in the constructor.
http://codereview.adblockplus.org/11756012/diff/1/src/engine/Main.cpp File src/engine/Main.cpp (right): http://codereview.adblockplus.org/11756012/diff/1/src/engine/Main.cpp#newcode386 src/engine/Main.cpp:386: Why this extra whitespace? http://codereview.adblockplus.org/11756012/diff/1/src/engine/Main.cpp#newcode407 src/engine/Main.cpp:407: Communication::browserSID = argv[2]; What if argc is <3? http://codereview.adblockplus.org/11756012/diff/1/src/plugin/AdblockPlusClien... File src/plugin/AdblockPlusClient.cpp (right): http://codereview.adblockplus.org/11756012/diff/1/src/plugin/AdblockPlusClien... src/plugin/AdblockPlusClient.cpp:4: #include <Sddl.h> Shouldn't these two includes go into PluginStdAfx? http://codereview.adblockplus.org/11756012/diff/1/src/plugin/AdblockPlusClien... src/plugin/AdblockPlusClient.cpp:37: acSid = (TOKEN_APPCONTAINER_INFORMATION*)HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, dwLength); Nit: Space before HeapAlloc? http://codereview.adblockplus.org/11756012/diff/1/src/plugin/AdblockPlusClien... src/plugin/AdblockPlusClient.cpp:38: if (acSid != NULL) What if the allocation failed? Isn't that worth an exception? http://codereview.adblockplus.org/11756012/diff/1/src/plugin/PluginClass.cpp File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/11756012/diff/1/src/plugin/PluginClass.cpp#... src/plugin/PluginClass.cpp:248: Not really related I think. http://codereview.adblockplus.org/11756012/diff/1/src/shared/Communication.cpp File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/11756012/diff/1/src/shared/Communication.cp... src/shared/Communication.cpp:38: bool GetLogonSid (HANDLE hToken, PSID *ppsid) Nit: No space after GetLogonSid? http://codereview.adblockplus.org/11756012/diff/1/src/shared/Communication.cp... src/shared/Communication.cpp:43: // Verify the parameter passed in is not NULL. I do think this is kinda obvious from the condition below. http://codereview.adblockplus.org/11756012/diff/1/src/shared/Communication.cp... src/shared/Communication.cpp:44: if (NULL == ppsid) We normally don't use Yoda conditions :D But it's fine if you prefer it that way. http://codereview.adblockplus.org/11756012/diff/1/src/shared/Communication.cp... src/shared/Communication.cpp:48: if (!GetTokenInformation(hToken, TokenLogonSid, (LPVOID) ptg, 0, &dwLength)) This code looks very similar to what's in AdblockPlusClient.cpp, can it be reused? Otherwise, the same comments apply here. http://codereview.adblockplus.org/11756012/diff/1/src/shared/Communication.cp... src/shared/Communication.cpp:87: BOOL bSuccess = FALSE; I'd rather have these declarations closer to their first use, and not in hungarian :)
I have tested on XP/IE6 - worked fine for me. There only one TODO in installer. Sorry I have messed up something with my queue. Parts of another changeset got into this one somehow. http://codereview.adblockplus.org/11756012/diff/1/src/engine/Main.cpp File src/engine/Main.cpp (right): http://codereview.adblockplus.org/11756012/diff/1/src/engine/Main.cpp#newcode407 src/engine/Main.cpp:407: Communication::browserSID = argv[2]; On 2013/09/16 16:30:12, Felix H. Dahlke wrote: > What if argc is <3? <3. Teehee :) http://codereview.adblockplus.org/11756012/diff/1/src/plugin/AdblockPlusClien... File src/plugin/AdblockPlusClient.cpp (right): http://codereview.adblockplus.org/11756012/diff/1/src/plugin/AdblockPlusClien... src/plugin/AdblockPlusClient.cpp:37: acSid = (TOKEN_APPCONTAINER_INFORMATION*)HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, dwLength); On 2013/09/16 16:30:12, Felix H. Dahlke wrote: > Nit: Space before HeapAlloc? "new" isn't good here since in the specific example the required number of bytes to allocate is 48. And sizeof(TOKEN_APPCONTAIER_INFORMATION) is not 48. Some sort of trickery would still have to be made even if auto_ptr was used.
Please go through the comments for Communication.cpp/h again, these haven't been addressed. http://codereview.adblockplus.org/11756012/diff/1/src/plugin/AdblockPlusClien... File src/plugin/AdblockPlusClient.cpp (right): http://codereview.adblockplus.org/11756012/diff/1/src/plugin/AdblockPlusClien... src/plugin/AdblockPlusClient.cpp:37: acSid = (TOKEN_APPCONTAINER_INFORMATION*)HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, dwLength); On 2013/09/17 03:11:37, Oleksandr wrote: > "new" isn't good here since in the specific example the required number of bytes > to allocate is 48. And sizeof(TOKEN_APPCONTAINER_INFORMATION) is not 48. What I actually meant: std::unique_ptr<char[]> sid(new char[length]); You can later cast the pointer to TOKEN_APPCONTAINER_INFORMATION when you need it. Note that unique_ptr will deal with arrays correctly, unlike auto_ptr. http://codereview.adblockplus.org/11756012/diff/11010/installer/bho_registry_... File installer/bho_registry_value.wxi (right): http://codereview.adblockplus.org/11756012/diff/11010/installer/bho_registry_... installer/bho_registry_value.wxi:29: TODO: Figure out how to write this key The class registration happens in dll_class.wxi. Unfortunately, WiX doesn't support this particular category yet so you'll have to write out the registry key "manually". Note that the installer should never write to HKCR, so it's rather the Software\CLASSES\CLSID\{FFCB3198-32F3-4E8B-9539-4324694ED664}\Implemented Categories\{59fb2056-d625-48d0-a944-1a85b5ab2640} key under HKLM. http://codereview.adblockplus.org/11756012/diff/11010/src/plugin/AdblockPlus.rgs File src/plugin/AdblockPlus.rgs (right): http://codereview.adblockplus.org/11756012/diff/11010/src/plugin/AdblockPlus.... src/plugin/AdblockPlus.rgs:3: AdblockPlus.AdblockPlus.1 = s 'AdblockPlus Class' Mind converting the tabs to spaces in this file? Currently the indentation looks off. http://codereview.adblockplus.org/11756012/diff/11010/src/plugin/AdblockPlus.... src/plugin/AdblockPlus.rgs:66: val 'AppPath' = s 'C:\Program Files\Adblock Plus for IE' I would prefer having the DLL pass in a custom parameter deduced from its own path. Not necessary for this release (custom install script parameters seem non-trivial) but should be addressed later. http://codereview.adblockplus.org/11756012/diff/11010/src/plugin/AdblockPlusC... File src/plugin/AdblockPlusClient.cpp (right): http://codereview.adblockplus.org/11756012/diff/11010/src/plugin/AdblockPlusC... src/plugin/AdblockPlusClient.cpp:26: TOKEN_APPCONTAINER_INFORMATION *acSid = NULL; Nit: that variable name also uses the Hungarian notation. http://codereview.adblockplus.org/11756012/diff/11010/src/plugin/AdblockPlusC... src/plugin/AdblockPlusClient.cpp:63: createProcRes = CreateProcessAsUser(newToken, engineExecutablePath.c_str(), params.GetBuffer(params.GetLength() + 1), Nit: This should be changed into CreateProcessAsUserW as well. http://codereview.adblockplus.org/11756012/diff/11010/src/shared/Communicatio... File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/11756012/diff/11010/src/shared/Communicatio... src/shared/Communication.cpp:1: #include <Windows.h> From what I can tell, none of my comments in this file have been addressed. http://codereview.adblockplus.org/11756012/diff/11010/src/shared/Communicatio... src/shared/Communication.cpp:202: Communication::Pipe::Pipe(const std::wstring& pipeName, Communication::Pipe::Mode mode) Not addressed: the empty line before this function should stay. http://codereview.adblockplus.org/11756012/diff/11010/src/shared/Communication.h File src/shared/Communication.h (right): http://codereview.adblockplus.org/11756012/diff/11010/src/shared/Communicatio... src/shared/Communication.h:194: PSECURITY_DESCRIPTOR securitydescriptor; My comment hasn't been addressed: Why do we need this as a member variable? It is only used in the constructor.
Comments (I think all) addressed in a commit. But I am not able to upload a codereview at the moment for some reason. http://codereview.adblockplus.org/11756012/diff/1/src/shared/Communication.cpp File src/shared/Communication.cpp (right): http://codereview.adblockplus.org/11756012/diff/1/src/shared/Communication.cp... src/shared/Communication.cpp:232: if (GetLogonSid(hToken, &pLogonSid) && CreateObjectSecurityDescriptor(pLogonSid, &securitydescriptor) ) On 2013/09/16 13:45:07, Wladimir Palant wrote: > Why do we need to allow the logon sid? Is it for testing only or can the plugin > run both inside and outside the app container? Our plugin is unable to access the named pipe if we don't allow logon sid http://codereview.adblockplus.org/11756012/diff/1/src/shared/Communication.h File src/shared/Communication.h (right): http://codereview.adblockplus.org/11756012/diff/1/src/shared/Communication.h#... src/shared/Communication.h:194: PSECURITY_DESCRIPTOR securitydescriptor; On 2013/09/16 13:45:07, Wladimir Palant wrote: > Why do we need this as a local variable? It is only used in the constructor. We need to be able to delete it when object is destructed http://codereview.adblockplus.org/11756012/diff/11010/src/plugin/AdblockPlus.rgs File src/plugin/AdblockPlus.rgs (right): http://codereview.adblockplus.org/11756012/diff/11010/src/plugin/AdblockPlus.... src/plugin/AdblockPlus.rgs:66: val 'AppPath' = s 'C:\Program Files\Adblock Plus for IE' On 2013/09/17 07:53:48, Wladimir Palant wrote: > I would prefer having the DLL pass in a custom parameter deduced from its own > path. Not necessary for this release (custom install script parameters seem > non-trivial) but should be addressed later. I agree with that. However, FYI, I don't think I've mentioned before that in order for IE in EPM to load our plugin at all - it has to be in Program Files folder. So some custom dev builds might fail here.
Seems mostly LGTM to me. Since you're on holiday now, I suppose I'll address any remaining issues. Anything left you'd like to see addressed, Wladimir? I'll try to delete that security descriptor after the constructor, other than that, seems fine. http://codereview.adblockplus.org/11756012/diff/1/src/shared/Communication.h File src/shared/Communication.h (right): http://codereview.adblockplus.org/11756012/diff/1/src/shared/Communication.h#... src/shared/Communication.h:194: PSECURITY_DESCRIPTOR securitydescriptor; On 2013/09/17 08:27:19, Oleksandr wrote: > On 2013/09/16 13:45:07, Wladimir Palant wrote: > > Why do we need this as a local variable? It is only used in the constructor. > We need to be able to delete it when object is destructed Shouldn't it have the same lifetime as the SECURITY_ATTRIBUTES instance we use in the constructor? That's how it was before. http://codereview.adblockplus.org/11756012/diff/11010/src/plugin/AdblockPlus.rgs File src/plugin/AdblockPlus.rgs (right): http://codereview.adblockplus.org/11756012/diff/11010/src/plugin/AdblockPlus.... src/plugin/AdblockPlus.rgs:66: val 'AppPath' = s 'C:\Program Files\Adblock Plus for IE' On 2013/09/17 08:27:19, Oleksandr wrote: > On 2013/09/17 07:53:48, Wladimir Palant wrote: > > I would prefer having the DLL pass in a custom parameter deduced from its own > > path. Not necessary for this release (custom install script parameters seem > > non-trivial) but should be addressed later. > I agree with that. However, FYI, I don't think I've mentioned before that in > order for IE in EPM to load our plugin at all - it has to be in Program Files > folder. So some custom dev builds might fail here. That's not great, will complicate development a bit, eh? More on topic: I don't think we can hard code this in the long term. Even if we're 100% sure it'll be in Program Files, it's not necessarily on the C drive. It's quite possible to use a different drive letter for the Windows partition.
On 2013/09/17 09:13:51, Felix H. Dahlke wrote: > Anything left you'd like to see addressed, Wladimir? I'll try to delete that > security descriptor after the constructor, other than that, seems fine. Category registration in the installer needs to be fixed as well. Other than that my comments are mostly nits but we should definitely get rid of HeapAlloc() and the corresponding memory leak before release. Same goes for GetLogonSid(), the logic should get significantly simpler if we don't have to clean up manually. What I still have a problem with are the Communication.cpp changes - that's a large chunk of code taken over from MSDN almost literally. There are licensing issues, I don't think that Microsoft Limited Public License (the license of MSDN examples) is even GPL-compatible. There are also readability issues - I hope that Oleksandr understands this code, I certainly have my troubles with it.
http://codereview.adblockplus.org/11756012/diff/11010/src/plugin/AdblockPlus.rgs File src/plugin/AdblockPlus.rgs (right): http://codereview.adblockplus.org/11756012/diff/11010/src/plugin/AdblockPlus.... src/plugin/AdblockPlus.rgs:66: val 'AppPath' = s 'C:\Program Files\Adblock Plus for IE' On 2013/09/17 09:13:51, Felix H. Dahlke wrote: > That's not great, will complicate development a bit, eh? Well, setting up the development environment should require elevation anyway - meaning that we can create a junction point under Program Files pointing to the build dir and register the DLL under that path. > More on topic: I don't think we can hard code this in the long term. Even if > we're 100% sure it'll be in Program Files, it's not necessarily on the C drive. Of course not, the Program Files directory can be anywhere. The hardcoded path here is only acceptable right now because it isn't used for anything but testing (the installer doesn't use the auto-registration).
I've reduced the security descriptor scope here: https://hg.adblockplus.org/adblockplusie/rev/4b4354e8d5eb On 2013/09/17 10:12:30, Wladimir Palant wrote: > Category registration in the installer needs to be fixed as well. Other than > that my comments are mostly nits but we should definitely get rid of HeapAlloc() > and the corresponding memory leak before release. Same goes for GetLogonSid(), > the logic should get significantly simpler if we don't have to clean up > manually. I thought all your comments where addressed by now. I guess I'll look through and see what's not... > What I still have a problem with are the Communication.cpp changes - that's a > large chunk of code taken over from MSDN almost literally. There are licensing > issues, I don't think that Microsoft Limited Public License (the license of MSDN > examples) is even GPL-compatible. There are also readability issues - I hope > that Oleksandr understands this code, I certainly have my troubles with it. I'll look into simplifying that stuff. Using RAII alone should make the code almost unrecognisable I think.
LGTM for https://hg.adblockplus.org/adblockplusie/rev/4b4354e8d5eb. I've also fixed the indentation in AdblockPlus.rgs under https://hg.adblockplus.org/adblockplusie/rev/8a8f173554ca - it was seriously messed up. Most issues noted in this review have been addressed apparently. Manual memory management with HeapAlloc() is still there however, also the TODO comment concerning registering the category in the installer. Setting sa.bInheritedHandle and sa.nLength in the Pipe constructor should always happen before the branching in the code - we always want to do that.
Changes so far: Only open the process token when needed, and close it afterwards https://hg.adblockplus.org/adblockplusie/rev/0c387378195d Refactor security attributes initialisation https://hg.adblockplus.org/adblockplusie/rev/4d2f4af69748 Register the Implemented Categories key in HKLM https://hg.adblockplus.org/adblockplusie/rev/73b2c6e11388 I'll test this on XP and then release a devbuild. But we still need to do something about GetLogonSid and CreateObjectSecurityDescriptor. They have a bunch of quality issues, and most importantly licensing issues.
On 2013/09/18 17:00:47, Felix H. Dahlke wrote: > Changes so far: > > Only open the process token when needed, and close it afterwards > https://hg.adblockplus.org/adblockplusie/rev/0c387378195d > > Refactor security attributes initialisation > https://hg.adblockplus.org/adblockplusie/rev/4d2f4af69748 > > Register the Implemented Categories key in HKLM > https://hg.adblockplus.org/adblockplusie/rev/73b2c6e11388 > > I'll test this on XP and then release a devbuild. > > But we still need to do something about GetLogonSid and > CreateObjectSecurityDescriptor. They have a bunch of quality issues, and most > importantly licensing issues. One more: Simplify conditions https://hg.adblockplus.org/adblockplusie/rev/45fe006e4de1
LGTM for https://hg.adblockplus.org/adblockplusie/rev/73b2c6e11388, https://hg.adblockplus.org/adblockplusie/rev/45fe006e4de1, https://hg.adblockplus.org/adblockplusie/rev/4d2f4af69748. https://hg.adblockplus.org/adblockplusie/rev/0c387378195d is fine but can't we use AutoHandle here? Overloading operator& in AutoHandle might work to allow it being used for out values.
On 2013/09/18 17:22:37, Wladimir Palant wrote: > LGTM for https://hg.adblockplus.org/adblockplusie/rev/73b2c6e11388, > https://hg.adblockplus.org/adblockplusie/rev/45fe006e4de1, > https://hg.adblockplus.org/adblockplusie/rev/4d2f4af69748. > https://hg.adblockplus.org/adblockplusie/rev/0c387378195d is fine but can't we > use AutoHandle here? Overloading operator& in AutoHandle might work to allow it > being used for out values. Done: Make AutoHandle usable in calls that expect a PHANDLE https://hg.adblockplus.org/adblockplusie/rev/9714027944a1 I think overloading operator& is a bit confusing here, shared_ptr e.g. doesn't do that either (those have a get function). I went for the PHANDLE cast operator as you suggested on IRC. Note that we can probably use AutoHandle in a bunch of places by now. I opted for not doing that refactoring later.
On 2013/09/19 08:20:12, Felix H. Dahlke wrote: > Done: > Make AutoHandle usable in calls that expect a PHANDLE > https://hg.adblockplus.org/adblockplusie/rev/9714027944a1 LGTM, very nice. > I think overloading operator& is a bit confusing here, I don't think C++ even supports overloading that operator, I suggested it without really thinking about it. > Note that we can probably use AutoHandle in a bunch of places by now. I opted > for not doing that refactoring later. Yes, we definitely should use it a lot more consistently.
Refactored GetLogonSid: https://hg.adblockplus.org/adblockplusie/rev/a97dd18a0de0
(Message was lost for some reason...) Refactored CreateObjectSecurityDescriptor https://hg.adblockplus.org/adblockplusie/rev/2e3c96683d85 Two remarks: 1. I'm returning a null pointer on error instead of throwing an exception. That's in line with the old behaviour (the return value was ignored), and in practice, errors do occur sometimes. 2. I'm not particularly happy with using shared_ptr just for cleanup, but it's the best I could come up with (auto_ptr doesn't support custom deleters). With this, all issues in this review should be addressed now. Except for any new ones with the refactoring, that is.
On 2013/09/21 16:06:53, Felix H. Dahlke wrote: > 2. I'm not particularly happy with using shared_ptr just for cleanup, but it's > the best I could come up with (auto_ptr doesn't support custom deleters). Without having seen the change yet, auto_ptr is pretty much deprecated. We should probably replace it by unique_ptr which does support custom deleters.
On 2013/09/23 06:12:23, Wladimir Palant wrote: > On 2013/09/21 16:06:53, Felix H. Dahlke wrote: > > 2. I'm not particularly happy with using shared_ptr just for cleanup, but it's > > the best I could come up with (auto_ptr doesn't support custom deleters). > > Without having seen the change yet, auto_ptr is pretty much deprecated. We > should probably replace it by unique_ptr which does support custom deleters. auto_ptr is deprecated in C++11 (in favour of unique_ptr), but we're using C++98 so far. The central difference is that auto_ptr's copy constructor moves ownership (somewhat hackily), while unique_ptr is movable using move semantics (which are introduced in C++11). Also, in case you're wondering why I got rid of so many LocalAlloc/HeapAlloc calls: Correct me (and the code) if I'm wrong, but they don't seem necessary. HeapAlloc seems to just make sense if you need to allocate memory on a different heap (but we didn't), and LocalAlloc is just a Windows specific allocator that uses HeapAlloc under the hood, similar to malloc. We can't free memory allocated with LocalAlloc using delete, so I'm still using Windows-specific free calls in some places. But where possible, I switched to new/delete.
On 2013/09/23 06:24:54, Felix H. Dahlke wrote: > On 2013/09/23 06:12:23, Wladimir Palant wrote: > > On 2013/09/21 16:06:53, Felix H. Dahlke wrote: > > > 2. I'm not particularly happy with using shared_ptr just for cleanup, but > it's > > > the best I could come up with (auto_ptr doesn't support custom deleters). > > > > Without having seen the change yet, auto_ptr is pretty much deprecated. We > > should probably replace it by unique_ptr which does support custom deleters. > > auto_ptr is deprecated in C++11 (in favour of unique_ptr), but we're using C++98 > so far. The central difference is that auto_ptr's copy constructor moves > ownership (somewhat hackily), while unique_ptr is movable using move semantics > (which are introduced in C++11). I forgot the conclusion: If we stick to C++98 with TR1 (just some C++98 compatible standard library additions like shared_ptr), we can use auto_ptr and shared_ptr. If we do move to C++11, we can use unique_ptr and shared_ptr. I'd vote for sticking with C++98 though, for portability's sake.
On 2013/09/23 06:24:54, Felix H. Dahlke wrote: > Also, in case you're wondering why I got rid of so many LocalAlloc/HeapAlloc > calls: Correct me (and the code) if I'm wrong, but they don't seem necessary. You are correct. The only scenario where we should use LocalAlloc/HeapAlloc is where a specific allocator is required by the Windows APIs (which is pretty uncommon).
On 2013/09/23 06:28:16, Felix H. Dahlke wrote: > I forgot the conclusion: If we stick to C++98 with TR1 (just some C++98 > compatible standard library additions like shared_ptr), we can use auto_ptr and > shared_ptr. If we do move to C++11, we can use unique_ptr and shared_ptr. According to http://gcc.gnu.org/onlinedocs/libstdc++/manual/status.html GCC supports unique_ptr as part of TR1. From the look of it, we can use it. Did I miss anything?
On 2013/09/23 09:24:06, Wladimir Palant wrote: > According to http://gcc.gnu.org/onlinedocs/libstdc++/manual/status.html GCC > supports unique_ptr as part of TR1. Ok, it seems that I'm mistaken. That document is very confusing. Too bad, I guess we shouldn't be using unique_ptr then...
Oleksandr fixed the Windows 8 hang in https://hg.adblockplus.org/adblockplusie/rev/49fd6bdb819f. Just for the record, LGTM from me for that change.
On 2013/09/25 08:41:18, Wladimir Palant wrote: > Oleksandr fixed the Windows 8 hang in > https://hg.adblockplus.org/adblockplusie/rev/49fd6bdb819f. Just for the record, > LGTM from me for that change. WTF, can't believe it. Let's not talk about that anymore, LGTM :) 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
