On Thu, Sep 04, 2014 at 06:55:46AM -0500, Tyler Hicks wrote: > Update the test server program to use socketpair() and pass the socket > fd to the client program. > > Make the test script aware of the special "none" keyword for unnamed > UNIX domain sockets. > > Signed-off-by: Tyler Hicks <[email protected]>
Acked-by: Seth Arnold <[email protected]> This is fine as it is but I've got a few suggestions inline: Thanks > --- > tests/regression/apparmor/unix_socket.c | 88 > +++++++++++++++++++------- > tests/regression/apparmor/unix_socket.sh | 48 +++++++++++--- > tests/regression/apparmor/unix_socket_client.c | 78 +++++++++++++++++------ > 3 files changed, 161 insertions(+), 53 deletions(-) > > diff --git a/tests/regression/apparmor/unix_socket.c > b/tests/regression/apparmor/unix_socket.c > index 50ae8e8..0e495a2 100644 > --- a/tests/regression/apparmor/unix_socket.c > +++ b/tests/regression/apparmor/unix_socket.c > @@ -14,6 +14,8 @@ > * along with this program; if not, contact Canonical Ltd. > */ > > +#define _GNU_SOURCE > + > #include <stdio.h> > #include <stdlib.h> > #include <string.h> > @@ -21,18 +23,24 @@ > #include <sys/types.h> > #include <sys/un.h> > #include <unistd.h> > +#include <fcntl.h> > > -#define MSG_BUF_MAX 1024 > +#define MSG_BUF_MAX 1024 > +#define PATH_FOR_UNNAMED "none" > > -static int connection_based_messaging(int sock, char *msg_buf, > - size_t msg_buf_len) > +static int connection_based_messaging(int sock, int sock_is_peer_sock, > + char *msg_buf, size_t msg_buf_len) > { > int peer_sock, rc; > > - peer_sock = accept(sock, NULL, NULL); > - if (peer_sock < 0) { > - perror("FAIL - accept"); > - return 1; > + if (sock_is_peer_sock) { > + peer_sock = sock; > + } else { > + peer_sock = accept(sock, NULL, NULL); > + if (peer_sock < 0) { > + perror("FAIL - accept"); > + return 1; > + } > } > > rc = write(peer_sock, msg_buf, msg_buf_len); > @@ -118,7 +126,8 @@ int main (int argc, char *argv[]) > const char *sun_path; > size_t sun_path_len; > pid_t pid; > - int sock, type, rc; > + int sock, peer_sock, type, rc; > + int unnamed = 0; > > if (argc != 5) { > fprintf(stderr, > @@ -138,6 +147,8 @@ int main (int argc, char *argv[]) > memcpy(addr.sun_path, sun_path, sun_path_len); > addr.sun_path[0] = '\0'; > sun_path_len = sizeof(addr.sun_path); > + } else if (!strcmp(sun_path, PATH_FOR_UNNAMED)) { > + unnamed = 1; > } else { > memcpy(addr.sun_path, sun_path, sun_path_len + 1); > } > @@ -160,25 +171,43 @@ int main (int argc, char *argv[]) > } > memcpy(msg_buf, argv[3], msg_buf_len); > > - sock = socket(AF_UNIX, type | SOCK_CLOEXEC, 0); > - if (sock == -1) { > - perror("FAIL - socket"); > - exit(1); > - } > + if (unnamed) { > + int sv[2]; > > - rc = bind(sock, (struct sockaddr *)&addr, > - sun_path_len + sizeof(addr.sun_family)); > - if (rc < 0) { > - perror("FAIL - bind"); > - exit(1); > - } > + rc = socketpair(AF_UNIX, type, 0, sv); > + if (rc == -1) { > + perror("FAIL - socketpair"); > + exit(1); > + } > + sock = sv[0]; > + peer_sock = sv[1]; > > - if (type & SOCK_STREAM || type & SOCK_SEQPACKET) { > - rc = listen(sock, 2); > + rc = fcntl(sock, F_SETFD, FD_CLOEXEC); > + if (rc == -1) { > + perror("FAIL - fcntl"); > + exit(1); > + } > + } else { > + sock = socket(AF_UNIX, type | SOCK_CLOEXEC, 0); > + if (sock == -1) { > + perror("FAIL - socket"); > + exit(1); > + } > + > + rc = bind(sock, (struct sockaddr *)&addr, > + sun_path_len + sizeof(addr.sun_family)); > if (rc < 0) { > - perror("FAIL - listen"); > + perror("FAIL - bind"); > exit(1); > } > + > + if (type & SOCK_STREAM || type & SOCK_SEQPACKET) { > + rc = listen(sock, 2); > + if (rc < 0) { > + perror("FAIL - listen"); > + exit(1); > + } > + } > } > > pid = fork(); > @@ -186,7 +215,18 @@ int main (int argc, char *argv[]) > perror("FAIL - fork"); > exit(1); > } else if (!pid) { > - execl(argv[4], argv[4], sun_path, argv[2], NULL); > + char *fd_number = NULL; > + > + if (unnamed) { > + rc = asprintf(&fd_number, "%d", peer_sock); > + if (rc == -1) { > + perror("FAIL - asprintf"); > + exit(1); > + } > + } > + > + /* fd_number will be NULL for pathname and abstract sockets */ > + execl(argv[4], argv[4], sun_path, argv[2], fd_number, NULL); > exit(0); If the execl() fails this should print a FAIL and exit(1). > } > > @@ -195,7 +235,7 @@ int main (int argc, char *argv[]) > exit(1); > > rc = (type & SOCK_STREAM || type & SOCK_SEQPACKET) ? > - connection_based_messaging(sock, msg_buf, msg_buf_len) : > + connection_based_messaging(sock, unnamed, msg_buf, msg_buf_len) > : > connectionless_messaging(sock, msg_buf, msg_buf_len); > if (rc) > exit(1); > diff --git a/tests/regression/apparmor/unix_socket.sh > b/tests/regression/apparmor/unix_socket.sh > index 7c23464..3878728 100755 > --- a/tests/regression/apparmor/unix_socket.sh > +++ b/tests/regression/apparmor/unix_socket.sh > @@ -34,6 +34,8 @@ sockpath_pathname=${tmpdir}/unix_socket.sock > bad_sockpath_pathname="${sockpath_pathname}XXX" > sockpath_abstract="@apparmor_unix_socket" > bad_sockpath_abstract="${sockpath_abstract}XXX" > +sockpath_unnamed="none" > +bad_sockpath_unnamed="$sockpath_abstract" > message=4a0c83d87aaa7afa2baab5df3ee4df630f0046d5bfb7a3080c550b721f401b3b\ > 8a738e1435a3b77aa6482a70fb51c44f20007221b85541b0184de66344d46a4c > > @@ -42,9 +44,14 @@ isabstract() > [ "${1:0:1}" == "@" ] > } > > +isunnamed() > +{ > + [ "$1" == "none" ] > +} > + > removesocket() > { > - if ! isabstract "$1"; then > + if ! isabstract "$1" && ! isunnamed "$1" ; then > rm -f "$1" > fi > } > @@ -63,7 +70,7 @@ testsocktype() > local okclients > local badclients > > - if isabstract $sockpath; then > + if isabstract "$sockpath" || isunnamed "$sockpath"; then > local ls_access # local server accesses > local ps_access # peer server accesses > local s_access # combined server accesses > @@ -74,14 +81,38 @@ testsocktype() > > local access # used for iterating accesses > > + local client_create # set if client create perms are needed > + local ps_sock_label # peer server socket label > + > + if isabstract "$sockpath"; then > + # unix_socket_client creates the socket which means: > + # 1) the create access is required > + # 2) the socket will be labeled as unconfined during > + # the confined server tests > + client_create="create," > + ps_sock_label="unconfined" > + else # unnamed > + # unix_socket_client inherits the socket which means: > + # 1) the create access is not needed > + # 2) the socket will be labeled as $test during the > + # confined server tests > + client_create="" > + ps_sock_label="$test" > + fi > + > if [ "$socktype" == "dgram" ]; then > # Connectionless > # Server doesn't listen() or accept() > ls_access="create,bind,getopt,setopt" > ps_access="read,write" > > - # Client calls bind() > - lc_access="${client_create}bind,getopt,setopt" > + lc_access="$client_create" > + # Client calls bind() if a connectionless pathname or > + # abstract socket is used > + if ! isunnamed "$sockpath"; then > + lc_access+="bind," > + fi > + lc_access+="getopt,setopt" > pc_access="connect,write,read" > else # stream or seqpacket > # Connection based > @@ -101,11 +132,11 @@ testsocktype() > "unix:($s_access)" \ > "unix:addr=$sockpath" \ > "unix:type=$socktype" \ > - "unix:peer=(label=unconfined)" \ > + "unix:peer=(label=$ps_sock_label)" \ > "unix:($s_access):addr=$sockpath" \ > - "unix:($ls_access):addr=$sockpath > unix:($ps_access):addr=$sockpath:peer=(label=unconfined)" \ > - "unix:($ls_access):type=$socktype:addr=$sockpath > unix:($ps_access):type=$socktype:addr=$sockpath:peer=(label=unconfined)" \ > - > "unix:type=$socktype:addr=$sockpath:peer=(label=unconfined)" \ > + "unix:($ls_access):addr=$sockpath > unix:($ps_access):addr=$sockpath:peer=(label=$ps_sock_label)" \ > + "unix:($ls_access):type=$socktype:addr=$sockpath > unix:($ps_access):type=$socktype:addr=$sockpath:peer=(label=$ps_sock_label)" \ > + > "unix:type=$socktype:addr=$sockpath:peer=(label=$ps_sock_label)" \ > ) > # Start with no accessess, then remove each access one-by-one > # from the list of server accesses, and then test bad > @@ -218,4 +249,5 @@ testsockpath() > testsockpath "pathname socket" "$sockpath_pathname" "$bad_sockpath_pathname" > if [ "$(have_features network/af_unix)" == "true" ] ; then > testsockpath "abstract socket" "$sockpath_abstract" > "$bad_sockpath_abstract" > + testsockpath "unnamed socket" "$sockpath_unnamed" > "$bad_sockpath_unnamed" > fi > diff --git a/tests/regression/apparmor/unix_socket_client.c > b/tests/regression/apparmor/unix_socket_client.c > index c0892cf..e7bdbed 100644 > --- a/tests/regression/apparmor/unix_socket_client.c > +++ b/tests/regression/apparmor/unix_socket_client.c > @@ -22,7 +22,8 @@ > #include <sys/un.h> > #include <unistd.h> > > -#define MSG_BUF_MAX 1024 > +#define MSG_BUF_MAX 1024 > +#define PATH_FOR_UNNAMED "none" > > static int connection_based_messaging(int sock) > { > @@ -44,17 +45,26 @@ static int connection_based_messaging(int sock) > return 0; > } > > -static int connectionless_messaging(int sock) > +static int connectionless_messaging(int sock, int unnamed) > { > struct sockaddr_un addr; > char msg_buf[MSG_BUF_MAX]; > int rc; > > - addr.sun_family = AF_UNIX; > - rc = bind(sock, (struct sockaddr *)&addr, sizeof(sa_family_t)); > - if (rc < 0) { > - perror("FAIL CLIENT - bind"); > - return 1; > + /** > + * Calling bind() on an unnamed socket results in autobinding in the > + * abstract namespace. That's not wanted since we're testing unnamed > + * sockets and not abstract sockets. Finally, there's no need to call > + * bind() since the two unnamed sockets from socketpair() are already > + * connected and read for IO. > + */ > + if (!unnamed) { > + addr.sun_family = AF_UNIX; > + rc = bind(sock, (struct sockaddr *)&addr, sizeof(sa_family_t)); Does addr need to be zeroed before use? > + if (rc < 0) { > + perror("FAIL CLIENT - bind"); > + return 1; > + } > } > > rc = write(sock, NULL, 0); > @@ -108,17 +118,25 @@ static int get_set_sock_io_timeo(int sock) > return 0; > } > > +static void usage(const char *name) > +{ This should probably have a FAIL output here, just to cover all bases. > + fprintf(stderr, "Usage: %s <socket> <type> [<fd_number>]\n\n" > + " type\t\tstream, dgram, or seqpacket\n" > + " fd_number\t\tfd number for inherited unnamed socket\n", > + name); > +} > + > int main(int argc, char *argv[]) > { > struct sockaddr_un peer_addr; > const char *sun_path; > size_t sun_path_len; > int sock, type, rc; > + int unnamed = 0; > + const char *fd_number = NULL; > > - if (argc != 3) { > - fprintf(stderr, "Usage: %s <socket> <type>\n\n" > - " type\t\tstream, dgram, or seqpacket\n", > - argv[0]); > + if (argc < 3 || argc > 4) { > + usage(argv[0]); > exit(1); > } > > @@ -131,6 +149,13 @@ int main(int argc, char *argv[]) > memcpy(peer_addr.sun_path, sun_path, sun_path_len); > peer_addr.sun_path[0] = '\0'; > sun_path_len = sizeof(peer_addr.sun_path); > + } else if (!strcmp(sun_path, PATH_FOR_UNNAMED)) { > + unnamed = 1; > + if (argc != 4) { > + usage(argv[0]); > + exit(1); > + } > + fd_number = argv[3]; > } else { > memcpy(peer_addr.sun_path, sun_path, sun_path_len + 1); > } > @@ -146,26 +171,37 @@ int main(int argc, char *argv[]) > exit(1); > } > > - sock = socket(AF_UNIX, type, 0); > - if (sock < 0) { > - perror("FAIL CLIENT - socket"); > - exit(1); > + if (unnamed) { > + rc = sscanf(fd_number, "%d", &sock); > + if (rc != 1) { > + perror("FAIL CLIENT - sscanf"); > + usage(argv[0]); > + exit(1); > + } > + } else { > + sock = socket(AF_UNIX, type, 0); > + if (sock < 0) { > + perror("FAIL CLIENT - socket"); > + exit(1); > + } > } > > rc = get_set_sock_io_timeo(sock); > if (rc) > exit(1); > > - rc = connect(sock, (struct sockaddr *)&peer_addr, > - sun_path_len + sizeof(peer_addr.sun_family)); > - if (rc < 0) { > - perror("FAIL CLIENT - connect"); > - exit(1); > + if (!unnamed) { > + rc = connect(sock, (struct sockaddr *)&peer_addr, > + sun_path_len + sizeof(peer_addr.sun_family)); > + if (rc < 0) { > + perror("FAIL CLIENT - connect"); > + exit(1); > + } > } > > rc = (type == SOCK_STREAM || type == SOCK_SEQPACKET) ? > connection_based_messaging(sock) : > - connectionless_messaging(sock); > + connectionless_messaging(sock, unnamed); > if (rc) > exit(1); > > -- > 2.1.0 > > > -- > AppArmor mailing list > [email protected] > Modify settings or unsubscribe at: > https://lists.ubuntu.com/mailman/listinfo/apparmor >
signature.asc
Description: Digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
