[Swan-dev] management of whack socket file descriptors

[D. Hugh Redelmeier]

The Coverity report (at the bottom of this message) is interesting.

It was triggered by e30778728ea01dacb051276d835648e0fda0618c which 
actually fixed a leak and did not introduce any new one (as far as I
know).

The flow of whack file descriptors is quite subtle and it seems that 
maintainers have added leaks (and perhaps the opposite -- use after
close) over the years.

The whack socket file descriptor enables logging from pluto to go back to 
the whack command.  Only logging relevant to that particular command goes 
back.  The whack command doesn't complete until the last whack socket file 
descriptor is closed in pluto.

- pluto has a whack socket for the duration of the processing of the
  whack command.  Mostly that processing is a single event handler.
  And then the whack socket file descriptor is closed.

- If the whack processing initiates negotiation, that negotiation will
  be represented by a new state object.  That state object will get a
  duplicate of the whack socket file descriptor.  That allows logging from 
  the negotiation to go back to the whack command that initiated it.

- a state owns one.  That one must be closed when the state is deleted 
  (and on certain other events)

- functions that create a state must ensure that the state gets its own 
  whack file descriptor.  Someone has to dup a descriptor so that the 
  state can own one.

Refine this with the concept of a null file descriptor.  For example,
some states cannot be traced back to a whack command and thus an
actual whack socket file descriptor doesn't exist.  Also, after a
certain number of retries, we "release" the whack (by closing the
whack socket file descriptor).

There, you have the basic outline.

Although fairly simple in concept, the path of whack socket file 
descriptors through the code has gotten too complicated to easily remember 
obligations.  Evidence: I've killed two leaks today. It took Coverity to 
find them. I think that we need at least a notation to document these 
obligations.

There are similar obligations for other resources and they too should
be documented in the code.

Unfortunately, C does not have a way of declaring those obligations.
Rust does.  Perhaps we should copy their notation, as stylized
comments.  Too bad they won't be enforced.  Historically, that kind of
comment seem to suffer bit rot over time.

I guess we could add a wrapper around whack socket file descriptor
handling.  We could make leaks and use-after-close detectable at
runtime.

File descriptor leaks should result in whack commands that don't 
terminate.  Since I'm unaware of complaints of this nature, it seems 
likely that the leaks are on rarely executed paths.  Or maybe I just don't 
hear the complaints.

Use-after-free would be observable in two different ways:

- the whack command being released too soon

- logging from one whack command being translated to another command.  Or 
  worse: appearing on a completely unrelated file or socket.

| From: scan-admin at coverity.com
| Resent-From: Antony Antony <antony at fern.phenome.org>
| To: antony at phenome.org
| Resent-To: swan-dev at lists.libreswan.org
| Date: Fri, 22 Jun 2018 08:31:58 +0000 (UTC)
| Resent-Date: Fri, 22 Jun 2018 10:35:18 +0200
| Subject: [Swan-dev] New Defects reported by Coverity Scan for
|     antonyantony/libreswan
| 
| Hi,
| 
| Please find the latest report on new defect(s) introduced to antonyantony/libreswan found with Coverity Scan.
| 
| 1 new defect(s) introduced to antonyantony/libreswan found with Coverity Scan.
| 60 defect(s), reported by Coverity Scan earlier, were marked fixed in the recent build analyzed by Coverity Scan.
| 
| New defect(s) Reported-by: Coverity Scan
| Showing 1 of 1 defect(s)
| 
| 
| ** CID 1470137:  Resource leaks  (RESOURCE_LEAK)
| /programs/pluto/ipsec_doi.c: 313 in ipsecdoi_replace()
| 
| 
| ________________________________________________________________________________________________________
| *** CID 1470137:  Resource leaks  (RESOURCE_LEAK)
| /programs/pluto/ipsec_doi.c: 313 in ipsecdoi_replace()
| 307     		policy = (c->policy & ~POLICY_IPSEC_MASK &
| 308     				~policy_del) | policy_add;
| 309     
| 310     		initiator = pick_initiator(c, policy);
| 311     		passert(!HAS_IPSEC_POLICY(policy));
| 312     		if (initiator != NULL) {
| >>>     CID 1470137:  Resource leaks  (RESOURCE_LEAK)
| >>>     Failing to save or close handle opened by "dup(st->st_whack_sock)" leaks it.
| 313     			(void) initiator(dup_any(st->st_whack_sock),
| 314     				st->st_connection, st, policy, try, st->st_import
| 315     #ifdef HAVE_LABELED_IPSEC
| 316     				, st->sec_ctx
| 317     #endif
| 318     				);
| 
| 
| ________________________________________________________________________________________________________
| To view the defects in Coverity Scan visit, https://u2389337.ct.sendgrid.net/wf/click?upn=08onrYu34A-2BWcWUl-2F-2BfV0V05UPxvVjWch-2Bd2MGckcRa-2F6FIQzoHE-2FeGfLjPxAHnMmI2-2BcDlBXS8eK3EDO-2F5Fjl6z3ueBPZWbG1BXBakNu80-3D_4HMcbsf14xheNUejmIGHntUcwhwkdYITNaUIUw0xLaqz0N8SaUULk05XZRHaJR5f4bhmTSF4djLcxwhhxI1cocKBXliSVDUUGAmdHiZpSUANxL3-2BGJViajaAekqgabDudscTDCIYE8y7iM98QSdn3KoVUXdlfokQ5p0msl9-2BygcK-2B5VZPQ-2FjomrScfQmv-2BSuj2Kqv1UthUNpPDvPcxUlrA-3D-3D
| 
|   To manage Coverity Scan email notifications for "antony at phenome.org", click https://u2389337.ct.sendgrid.net/wf/click?upn=08onrYu34A-2BWcWUl-2F-2BfV0V05UPxvVjWch-2Bd2MGckcRbVDbis712qZDP-2FA8y06Nq4T11LJsTCGAu8UQysrZAC89q-2FP-2B3fe1J15ziye5UyoPhoOtByWqcc6ADp6Tx8rz1FuEBlJrervxOcvY4aXgJd53T3KDIOTHFcOCaGp5NkyaQ-3D_4HMcbsf14xheNUejmIGHntUcwhwkdYITNaUIUw0xLaqz0N8SaUULk05XZRHaJR5feO8VlTIYOkX8d29cjB3KQX9etyqOYxzJJEyAFBy0860rZHmjaqkcFmPmSti474O4Gftkll9kwHQ7Z-2FYOCkTJTTg4MWXZlK5hJfTjRjlxHK3IS7KcwEG2xwebthSeqKQHr-2Bcd2-2FwZX9Cw52g0QHi-2BtQ-3D-3D