Re: [PATCH 5/8] reconfiguration leaks: objects tied to http_port
On 13/07/2014 4:59 p.m., Amos Jeffries wrote: > On 23/06/2014 12:30 a.m., Amos Jeffries wrote: >> On 17/06/2014 9:15 a.m., Alex Rousskov wrote: >>> >>> PortCfgPointer is not a reference counting pointer. >> >> There is no remaining reason for that since we converted the TcpAcceptor >> to emitting MasterXaction. The PortCfg pointer is not actually passed as >> a parameter anywhere. Just one buggy piece of code which should have >> been implemented differently. >> >> >> >>> >>> Most likely, we should use the refcounting API for port pointers. Until >>> that (or a better) solution is implemented, we should either >>> >> >> The attached (rough) patch converts the PortCfgPointer to reference >> counted and fixes all parsing errors resulting from the change. Most of >> the issues were due to use of raw-pointers and explicit >> cbdataReference*() API. >> >> Still have to add stubs to fix make check linkage errors and do some run >> testing. >> > > If there are no objections I will commit the slightly more polished > version of this patch shortly. > > Amos > Applied. SSL state details may still be leaking, but I no longer see any issue with SSL cleanup patches being applied so long as the relevant cleanup is performed via the AnyP::PortCfg destructor. Amos
Re: [PATCH 5/8] reconfiguration leaks: objects tied to http_port
On 23/06/2014 12:30 a.m., Amos Jeffries wrote: > On 17/06/2014 9:15 a.m., Alex Rousskov wrote: >> >> PortCfgPointer is not a reference counting pointer. > > There is no remaining reason for that since we converted the TcpAcceptor > to emitting MasterXaction. The PortCfg pointer is not actually passed as > a parameter anywhere. Just one buggy piece of code which should have > been implemented differently. > > > >> >> Most likely, we should use the refcounting API for port pointers. Until >> that (or a better) solution is implemented, we should either >> > > The attached (rough) patch converts the PortCfgPointer to reference > counted and fixes all parsing errors resulting from the change. Most of > the issues were due to use of raw-pointers and explicit > cbdataReference*() API. > > Still have to add stubs to fix make check linkage errors and do some run > testing. > If there are no objections I will commit the slightly more polished version of this patch shortly. Amos
Re: [PATCH 5/8] reconfiguration leaks: objects tied to http_port
On 06/15/2014 12:02 AM, Amos Jeffries wrote: > On 15/06/2014 4:44 a.m., Alex Rousskov wrote: >> On 06/13/2014 07:25 PM, Amos Jeffries wrote: >>> On 14/06/2014 8:07 a.m., Alex Rousskov wrote: On 04/25/2014 02:59 AM, Amos Jeffries wrote: > On 25/04/2014 12:55 p.m., Alex Rousskov wrote: >> Do not leak [SSL] objects tied to http_port and https_port on >> reconfigure. >> >> PortCfg objects were not destroyed at all (no delete call) and were >> incorrectly stored (excessive cbdata locking). This change adds >> destruction and removes excessive locking to allow the destructed >> object to be freed. It also cleans up forgotten(?) clientca and crlfile >> PortCfg members. >> >> This change fixes a serious leak but also carries an elevated risk: >> There is a lot of code throughout Squid that does not check the pointers >> to the objects that are now properly destroyed. It is possible that some >> of that code will crash some time after reconfigure. It is not possible >> to ensure that this does not happen without rewriting/fixing the >> offending code to use refcounting. Such a rewrite would be a relatively >> large change outside this patch scope. We may decide that it is better >> to leak than to take this additional risk. > -0. > > I have a patch moving the SSL config options into a standalone > ref-counted object. That can be polished up and references added to each > ConnStateData fairly easily. Amos, what is the status of that patch? Any ETA? >>> Stalled behind the larger works. If it is urgent I can did it out and >>> polish it up. >>> >>> [...] The design is a new >>> Ref-Countable class to hold all the SSL options (and generated state) >>> leaving just a Pointer to it in the main config class. >>> * Ports which needed a clone operation took a copy of the pointer and >>> share the context. >>> * client/server context initialization functions take a Pointer to the >>> class and update its state content. >> Why treat SSL options differently from all other port options? Should >> not the whole PortCfg object be refcounted instead? > The other options are POD-style options. They can be reset relatively > okay by the parsers memset() and are filled out by the config parse > routines. AFAICT, not all other options are "POD-style options that can be reset relatively okay by memset()" unless "relatively OK" means "small leaks are OK". Some non-SSL port options require specialized cleanup just like some SSL port options do (PortCfg::name and defaultsite are two examples with listenConn arguably being the third). Thus, the requirement for specialized cleanup is not a reason to treat SSL options differently from all other port options. > Also, the same set of config settings are needed by *_port, cache_peer, > and sslproxy_* directives. It makes sense to have one class type for SSL > config which is used by all three. While it may indeed be a good idea to group related options together, such grouping is irrelevant for the purpose of this discussion. Whether we group SSL-related options or not, they and other port options will continue to leak. They leak not because they are not grouped but because Squid leaks the Port structure they belong to (grouped or not). > On the other side AnyP::PortCfgPointer already exists to reference count > the port objects. We could spend time converting the raw pointers to use > it instead so your patch here is not dangerous. PortCfgPointer is not a reference counting pointer. It is a cbdata-protected pointer. The proposed patch _is_ using/adding the associated cdbdata protections in/to some users (but more work may be needed to make all users safe, as disclosed). Simply converting all raw port pointers to use PortCfgPointer is not going to make us much safer (it will just make triaging crashes easier). The callers need to be changed to check that the pointer is still valid before dereferencing it. Unlike refcounting pointers, cbdata pointers do not protect naive code from crashing. The cbdata interface is also asymmetric -- there supposed to be only one object owner. Cbdata pointers are like std::weak_ptr and refcounting pointers are like std::shared_pointer. Very different APIs, offering different trade-offs. Most likely, we should use the refcounting API for port pointers. Until that (or a better) solution is implemented, we should either * stop port leaks or * acknowledge that we would rather see Squid leaking than increasing the probability of a Squid crash after reconfigure. Both approaches are reasonable. The proposed patch does the former. I do not want to commit that patch because there is only one vote and it is "-0". What bothers me here is that the justification for the -0 vote was (AFAICT) a promise of a better patch, but it now appears that the better patch is not coming soon, has problems of its own, and may not even remove the leak. I cannot really
Re: [PATCH 5/8] reconfiguration leaks: objects tied to http_port
On 15/06/2014 4:44 a.m., Alex Rousskov wrote: > On 06/13/2014 07:25 PM, Amos Jeffries wrote: >> On 14/06/2014 8:07 a.m., Alex Rousskov wrote: >>> On 04/25/2014 02:59 AM, Amos Jeffries wrote: On 25/04/2014 12:55 p.m., Alex Rousskov wrote: > Do not leak [SSL] objects tied to http_port and https_port on reconfigure. > > PortCfg objects were not destroyed at all (no delete call) and were > incorrectly stored (excessive cbdata locking). This change adds > destruction and removes excessive locking to allow the destructed > object to be freed. It also cleans up forgotten(?) clientca and crlfile > PortCfg members. > > This change fixes a serious leak but also carries an elevated risk: > There is a lot of code throughout Squid that does not check the pointers > to the objects that are now properly destroyed. It is possible that some > of that code will crash some time after reconfigure. It is not possible > to ensure that this does not happen without rewriting/fixing the > offending code to use refcounting. Such a rewrite would be a relatively > large change outside this patch scope. We may decide that it is better > to leak than to take this additional risk. > > Alex. > -0. I have a patch moving the SSL config options into a standalone ref-counted object. That can be polished up and references added to each ConnStateData fairly easily. >>> >>> Amos, what is the status of that patch? Any ETA? Do you expect your >>> changes to be easily portable to v3.3? >> >> Stalled behind the larger works. If it is urgent I can did it out and >> polish it up. >> >> It could be back-ported to 3.3 if you like. The design is a new >> Ref-Countable class to hold all the SSL options (and generated state) >> leaving just a Pointer to it in the main config class. >> * Ports which needed a clone operation took a copy of the pointer and >> share the context. >> * client/server context initialization functions take a Pointer to the >> class and update its state content. > > Why treat SSL options differently from all other port options? Should > not the whole PortCfg object be refcounted instead? > > Please note that if you refcount the SSL options and do nothing else, > the leak will not be fixed. The SSL options (and other things) will > continue to leak because the leaking port structure will keep pointing > to them. Thus, as far as I can see, the patch you described will need > the proposed leak fix anyway. The other options are POD-style options. They can be reset relatively okay by the parsers memset() and are filled out by the config parse routines. SSL options are partially those type of options and partially SSL library objects generated from those options. Generated options should be in SquidConfig2 or one of the new standalone config gobals. Also, the same set of config settings are needed by *_port, cache_peer, and sslproxy_* directives. It makes sense to have one class type for SSL config which is used by all three. On the other side AnyP::PortCfgPointer already exists to reference count the port objects. We could spend time converting the raw pointers to use it instead so your patch here is not dangerous. Amos
Re: [PATCH 5/8] reconfiguration leaks: objects tied to http_port
On 06/13/2014 07:25 PM, Amos Jeffries wrote: > On 14/06/2014 8:07 a.m., Alex Rousskov wrote: >> On 04/25/2014 02:59 AM, Amos Jeffries wrote: >>> On 25/04/2014 12:55 p.m., Alex Rousskov wrote: Do not leak [SSL] objects tied to http_port and https_port on reconfigure. PortCfg objects were not destroyed at all (no delete call) and were incorrectly stored (excessive cbdata locking). This change adds destruction and removes excessive locking to allow the destructed object to be freed. It also cleans up forgotten(?) clientca and crlfile PortCfg members. This change fixes a serious leak but also carries an elevated risk: There is a lot of code throughout Squid that does not check the pointers to the objects that are now properly destroyed. It is possible that some of that code will crash some time after reconfigure. It is not possible to ensure that this does not happen without rewriting/fixing the offending code to use refcounting. Such a rewrite would be a relatively large change outside this patch scope. We may decide that it is better to leak than to take this additional risk. Alex. >>> >>> -0. >>> >>> I have a patch moving the SSL config options into a standalone >>> ref-counted object. That can be polished up and references added to each >>> ConnStateData fairly easily. >> >> Amos, what is the status of that patch? Any ETA? Do you expect your >> changes to be easily portable to v3.3? > > Stalled behind the larger works. If it is urgent I can did it out and > polish it up. > > It could be back-ported to 3.3 if you like. The design is a new > Ref-Countable class to hold all the SSL options (and generated state) > leaving just a Pointer to it in the main config class. > * Ports which needed a clone operation took a copy of the pointer and > share the context. > * client/server context initialization functions take a Pointer to the > class and update its state content. Why treat SSL options differently from all other port options? Should not the whole PortCfg object be refcounted instead? Please note that if you refcount the SSL options and do nothing else, the leak will not be fixed. The SSL options (and other things) will continue to leak because the leaking port structure will keep pointing to them. Thus, as far as I can see, the patch you described will need the proposed leak fix anyway. Cheers, Alex.
Re: [PATCH 5/8] reconfiguration leaks: objects tied to http_port
On 14/06/2014 8:07 a.m., Alex Rousskov wrote: > On 04/25/2014 02:59 AM, Amos Jeffries wrote: >> On 25/04/2014 12:55 p.m., Alex Rousskov wrote: >>> Do not leak [SSL] objects tied to http_port and https_port on reconfigure. >>> >>> PortCfg objects were not destroyed at all (no delete call) and were >>> incorrectly stored (excessive cbdata locking). This change adds >>> destruction and removes excessive locking to allow the destructed >>> object to be freed. It also cleans up forgotten(?) clientca and crlfile >>> PortCfg members. >>> >>> This change fixes a serious leak but also carries an elevated risk: >>> There is a lot of code throughout Squid that does not check the pointers >>> to the objects that are now properly destroyed. It is possible that some >>> of that code will crash some time after reconfigure. It is not possible >>> to ensure that this does not happen without rewriting/fixing the >>> offending code to use refcounting. Such a rewrite would be a relatively >>> large change outside this patch scope. We may decide that it is better >>> to leak than to take this additional risk. >>> >>> Alex. >>> >> >> -0. >> >> I have a patch moving the SSL config options into a standalone >> ref-counted object. That can be polished up and references added to each >> ConnStateData fairly easily. > > Amos, what is the status of that patch? Any ETA? Do you expect your > changes to be easily portable to v3.3? Stalled behind the larger works. If it is urgent I can did it out and polish it up. It could be back-ported to 3.3 if you like. The design is a new Ref-Countable class to hold all the SSL options (and generated state) leaving just a Pointer to it in the main config class. * Ports which needed a clone operation took a copy of the pointer and share the context. * client/server context initialization functions take a Pointer to the class and update its state content. Amos
Re: [PATCH 5/8] reconfiguration leaks: objects tied to http_port
On 04/25/2014 02:59 AM, Amos Jeffries wrote: > On 25/04/2014 12:55 p.m., Alex Rousskov wrote: >> Do not leak [SSL] objects tied to http_port and https_port on reconfigure. >> >> PortCfg objects were not destroyed at all (no delete call) and were >> incorrectly stored (excessive cbdata locking). This change adds >> destruction and removes excessive locking to allow the destructed >> object to be freed. It also cleans up forgotten(?) clientca and crlfile >> PortCfg members. >> >> This change fixes a serious leak but also carries an elevated risk: >> There is a lot of code throughout Squid that does not check the pointers >> to the objects that are now properly destroyed. It is possible that some >> of that code will crash some time after reconfigure. It is not possible >> to ensure that this does not happen without rewriting/fixing the >> offending code to use refcounting. Such a rewrite would be a relatively >> large change outside this patch scope. We may decide that it is better >> to leak than to take this additional risk. >> >> Alex. >> > > -0. > > I have a patch moving the SSL config options into a standalone > ref-counted object. That can be polished up and references added to each > ConnStateData fairly easily. Amos, what is the status of that patch? Any ETA? Do you expect your changes to be easily portable to v3.3? Thank you, Alex.
Re: [PATCH 5/8] reconfiguration leaks: objects tied to http_port
On 25/04/2014 12:55 p.m., Alex Rousskov wrote: > Do not leak [SSL] objects tied to http_port and https_port on reconfigure. > > PortCfg objects were not destroyed at all (no delete call) and were > incorrectly stored (excessive cbdata locking). This change adds > destruction and removes excessive locking to allow the destructed > object to be freed. It also cleans up forgotten(?) clientca and crlfile > PortCfg members. > > This change fixes a serious leak but also carries an elevated risk: > There is a lot of code throughout Squid that does not check the pointers > to the objects that are now properly destroyed. It is possible that some > of that code will crash some time after reconfigure. It is not possible > to ensure that this does not happen without rewriting/fixing the > offending code to use refcounting. Such a rewrite would be a relatively > large change outside this patch scope. We may decide that it is better > to leak than to take this additional risk. > > Alex. > -0. I have a patch moving the SSL config options into a standalone ref-counted object. That can be polished up and references added to each ConnStateData fairly easily. Amos
