Add fallback for unsupported abstract sockets #278

aloisklink · 2022-09-19T14:45:25Z

Currently, create_domain_client(NULL) creates a new temporary abstract Unix domain socket:

Abstract sockets

[...]
Abstract sockets automatically disappear when all open references to the socket are closed.

The abstract socket namespace is a nonportable Linux extension.

(from https://manpages.ubuntu.com/manpages/jammy/en/man7/unix.7.html)

Since abstract sockets are "a notportable Linux extension", they are not supported by FreeBSD.

Because of this, I've instead replaced this code with creating a new pathname Unix domain socket, where the socket is located in /tmp/edgesec.XXXXXX/client-socket.sock, where /tmp/edgesec.XXXXXX is made with mkdtemp.

By default, this code is only used on non-Linux platforms, like FreeBSD.

I then added a new function called close_domain_socket() that:

closes Unix domain sockets, and
unlink() the pathname (only for _pathname_ Unix domain sockets), and
rmdir() the /tmp/edgesec.XXXXXX (only if the socket was created by create_domain_client(NULL) (I'm using a uthash map to confirm this)).

Add docs describing that generate_socket_name() returns a pseudo-random _abstract_ socket name, without the leading ␀-char.

Add details on what _abstract_ Unix domain sockets are, and that they're used when the input path is NULL.

These input variables are not changed, so we should make them const.

Combines variable definition and initialization, since we aren't using an old C89 compiler. Additionally, I've replaced all of the sockaddr_un initialisations with portable C default initializers, as memset() is explicitly not recommended as it isn't portable, see [manpage for netinet_in.h under APPLICATION USAGE][1] [1]: https://manpages.ubuntu.com/manpages/jammy/en/man7/netinet_in.h.7posix.html

Instead of creating our own abstract address, using 8 random hex chars, we can use Linux's autobind feature to create a new **unused** abstract address consisting of 5 random hex chars. This is 3 less hex chars, however Linux's autobind feature ensures that the a new abstract address is always picked, unlike our custom generator, which may rarely create an abstract address that is already being used. See https://manpages.ubuntu.com/manpages/jammy/en/man7/unix.7.html for documentation on the "autobind feature".

Creates a helper function that can be used to: - close a unix domain socket, and - unlink() the unix domain socket (if _pathname_ unix domain socket) I've modified the `close()` function after every create_domain_client(NULL) to instead call `close_domain_socket()`.

codecov · 2022-09-19T14:49:06Z

Codecov Report

Merging #278 (4674bae) into main (abdfe16) will increase coverage by 0.05%.
The diff coverage is 71.00%.

❗ Current head 4674bae differs from pull request most recent head ae16f1c. Consider uploading reports for the commit ae16f1c to get more accurate results

@@            Coverage Diff             @@
##             main     #278      +/-   ##
==========================================
+ Coverage   49.18%   49.24%   +0.05%     
==========================================
  Files         116      116              
  Lines       18574    18635      +61     
==========================================
+ Hits         9136     9177      +41     
- Misses       9438     9458      +20

Impacted Files	Coverage Δ
src/utils/sockctl.c	`48.23% <56.06%> (-0.88%)`	⬇️
src/ap/ap_service.c	`39.88% <100.00%> (ø)`
src/dns/mdns_service.c	`16.15% <100.00%> (ø)`
tests/supervisor/test_sockctl_server.c	`93.90% <100.00%> (+1.18%)`	⬆️
tests/ap/test_ap_service.c	`90.24% <0.00%> (-3.66%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

src/utils/sockctl.c

mereacre · 2022-09-19T14:54:29Z

src/utils/sockctl.c

 #define DOMAIN_REPLY_TIMEOUT 10

 void init_domain_addr(struct sockaddr_un *unaddr, const char *addr) {
  *unaddr = (struct sockaddr_un){.sun_family = AF_UNIX};
  os_strlcpy(unaddr->sun_path, addr, sizeof(unaddr->sun_path));
 }
+struct tmp_domain_socket {


Could you please explain this structure?

How are these doxygen docs? (added in b90bc2d)

Essentially, it's a hashmap struct that stores all the temporary unix domain sockets (and the folder they are in) created by create_tmp_domain_socket_path() so that the sockets and the folders can be properly deleted by cleanup_tmp_domain_socket_path()

edgesec/src/utils/sockctl.c

Lines 38 to 54 in b90bc2d

/**

* @brief uthash hashmap entry storing temporary pathname unix domain sockets.

*

* This data structure is created by create_tmp_domain_socket_path(),

* and should be deleted by calling cleanup_tmp_domain_socket_path().

*

* Only used if abstract unix domain sockets are not supported (e.g. on

* FreeBSD).

*/

struct tmp_domain_socket {

/** Full path to the socket. Hashmap key. */

char socket_path[sizeof(TMP_UNIX_SOCK_FOLDER_TEMPLATE) +

sizeof(TMP_UNIX_SOCK_NAME)];

/** The socket folder created by mkdtemp() */

char socket_folder[sizeof(TMP_UNIX_SOCK_FOLDER_TEMPLATE)];

/** makes this structure hashable by uthash */

UT_hash_handle hh;

We are using here a global variable. In general I prefer not to use any in the edgesec code. I think we should refactor and add things like context or similar. There's also the problem of thread safety. We haven't done any locking for the hash string. Check the thread safety section in https://troydhanson.github.io/uthash/userguide.html

We are using here a global variable. In general I prefer not to use any in the edgesec code.

Ahhh, that will make things difficult. I agree, global variables are a nightmare. I only implemented it that way to change as little as I could as possible.

It's a pity that in C you often have to use global variables, especially for stuff like signal handling.

Hmmmmm, in that case, I need to think of the best way of how to handle cleanup. How about:

Return a struct unix_socket {int fd; (void *)(void) destructor;} from create_domain_client(), so we can return a custom destructor() function (C++ style)

Just ignore cleaning up the domain sockets.

They're only used in FreeBSD

If we stick them all into a /tmp/edgesec/tmp-sockets.XXXXXX/*, we can just not bother cleaning them up, and as long it takes at years for 1 million tmp files to be created, FreeBSD will eventually shutdown and delete the /tmp folder.

I think the best answer is to ignore cleaning up the tmp files.

I've changed the way it works in ba72733

I've removed all of the UT_hash stuff and removed all the global variables.

Essentially, if any socket path starts with /tmp/edgesec., it deletes the socket and the parent dir.

For example: cleanup for /tmp/edgesec.XYZXYZ/client-socket.sock will do:

unlink("/tmp/edgesec.XYZXYZ/client-socket.sock");

rmdir("/tmp/edgesec.XYZXYZ");

Potential issues:

If we ever make something like /tmp/edgesec.example/my-custom-socket.sock, this might also try to delete the /tmp/edgesec.example folder. I don't think this is a major issue, but we could change the mkdtemp prefix to something like /tmp/edgesec/tmp-unix-sockets.XXXXXX if you want.

Could you please add a log_warn or similar saying which files are being deleted?

Added in ae16f1c

I've made it is only a log_debug(). This is because if we do try to delete the wrong folder, chances are that it's not empty and so rmdir() will have a ENOTEMPTY (dir not empty) error, and we'll get a log_errno() message.

…ockets

Currently, creating a temporary unix domain socket using `create_domain_client(NULL)` creates an abstract Unix domain socket, which only works on Linux. In order to support FreeBSD (and other Unix not-Linux platforms), `create_domain_client(NULL)` can now fallback to creating a new _pathname_ unix domain socket using `mkdtemp()`.

There's a very tiny chance that we'll misidentify a folder as a mkdtemp folder created by `create_tmp_domain_socket_path()`. For this case, I've added a log statement. It's only a log_debug(), since we expect that if this ever does misidentify a folder, rmdir() will throw an ENOTEMPTY (dir not empty) error, which will print an error log message.

aloisklink added 6 commits September 19, 2022 10:30

docs(sockctl): document generate_socket_name()

1ce4e17

Add docs describing that generate_socket_name() returns a pseudo-random _abstract_ socket name, without the leading ␀-char.

docs(sockctl): improve create_domain_client docs

338c641

Add details on what _abstract_ Unix domain sockets are, and that they're used when the input path is NULL.

refactor(socketctl): make domain params const

b00f855

These input variables are not changed, so we should make them const.

aloisklink added the bug Something isn't working label Sep 19, 2022

aloisklink requested a review from mereacre September 19, 2022 14:45

aloisklink added this to the CheriBSD Support milestone Sep 19, 2022

mereacre reviewed Sep 19, 2022

View reviewed changes

aloisklink added 3 commits September 20, 2022 15:29

Merge branch 'main' into feat/add-fallback-for-unsupported-abstract-s…

f34c098

…ockets

refactor(sockctl): suppress C compiler warnings

9ccb5ad

aloisklink force-pushed the feat/add-fallback-for-unsupported-abstract-sockets branch from 4674bae to 9ccb5ad Compare September 20, 2022 15:34

mereacre approved these changes Sep 20, 2022

View reviewed changes

aloisklink enabled auto-merge September 20, 2022 16:02

aloisklink merged commit f7173e4 into main Sep 20, 2022

aloisklink deleted the feat/add-fallback-for-unsupported-abstract-sockets branch September 20, 2022 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fallback for unsupported abstract sockets #278

Add fallback for unsupported abstract sockets #278

aloisklink commented Sep 19, 2022

codecov bot commented Sep 19, 2022 •

edited

Loading

mereacre Sep 19, 2022

aloisklink Sep 19, 2022

mereacre Sep 20, 2022

aloisklink Sep 20, 2022

mereacre Sep 20, 2022

aloisklink Sep 20, 2022 •

edited

Loading

mereacre Sep 20, 2022

aloisklink Sep 20, 2022

	/**
	* @brief uthash hashmap entry storing temporary pathname unix domain sockets.
	*
	* This data structure is created by create_tmp_domain_socket_path(),
	* and should be deleted by calling cleanup_tmp_domain_socket_path().
	*
	* Only used if abstract unix domain sockets are not supported (e.g. on
	* FreeBSD).
	*/
	struct tmp_domain_socket {
	/** Full path to the socket. Hashmap key. */
	char socket_path[sizeof(TMP_UNIX_SOCK_FOLDER_TEMPLATE) +
	sizeof(TMP_UNIX_SOCK_NAME)];
	/** The socket folder created by mkdtemp() */
	char socket_folder[sizeof(TMP_UNIX_SOCK_FOLDER_TEMPLATE)];
	/** makes this structure hashable by uthash */
	UT_hash_handle hh;

Add fallback for unsupported abstract sockets #278

Add fallback for unsupported abstract sockets #278

Conversation

aloisklink commented Sep 19, 2022

codecov bot commented Sep 19, 2022 • edited Loading

Codecov Report

mereacre Sep 19, 2022

Choose a reason for hiding this comment

aloisklink Sep 19, 2022

Choose a reason for hiding this comment

mereacre Sep 20, 2022

Choose a reason for hiding this comment

aloisklink Sep 20, 2022

Choose a reason for hiding this comment

mereacre Sep 20, 2022

Choose a reason for hiding this comment

aloisklink Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

mereacre Sep 20, 2022

Choose a reason for hiding this comment

aloisklink Sep 20, 2022

Choose a reason for hiding this comment

codecov bot commented Sep 19, 2022 •

edited

Loading

aloisklink Sep 20, 2022 •

edited

Loading