setsockopt() can not remove the accept filter

[Maxim Konovalov]

[ CC'ed rwatson ]

On Sat, 11 Jun 2005, 02:12+0400, Igor Sysoev wrote:

> Hi,
>
> man setsockopt(2) states that "passing in an optval of NULL will remove
> the filter", however, setsockopt() always return EINVAL in this case,
> because do_setopt_accept_filter() removes the filter if sopt == NULL, but
> not if sopt->val == NULL.  The fix is easy:
>
> -        if (sopt == NULL) {
> +        if (sopt == NULL || sopt->val == NULL) {

Yep, your observation is correct.  The second (mis)feature:
getsockopt(SO_ACCEPTFILTER) always returns success on listen socket
even we didn't install accept filter on the socket.

I extended the regression test for accept filter for these bugs.
That makes the name of the test misleading though because now it tests
detach case as well.

Index: sys/kern/uipc_accf.c
===================================================================
RCS file: /home/ncvs/src/sys/kern/uipc_accf.c,v
retrieving revision 1.18
diff -u -p -r1.18 uipc_accf.c
--- sys/kern/uipc_accf.c	12 Mar 2005 12:57:17 -0000	1.18
+++ sys/kern/uipc_accf.c	11 Jun 2005 06:53:47 -0000
@@ -176,8 +176,10 @@ do_getopt_accept_filter(struct socket *s
 		error = EINVAL;
 		goto out;
 	}
-	if ((so->so_options & SO_ACCEPTFILTER) == 0)
+	if ((so->so_options & SO_ACCEPTFILTER) == 0) {
+		error = EINVAL;
 		goto out;
+	}
 	strcpy(afap->af_name, so->so_accf->so_accept_filter->accf_name);
 	if (so->so_accf->so_accept_filter_str != NULL)
 		strcpy(afap->af_arg, so->so_accf->so_accept_filter_str);
@@ -200,7 +202,7 @@ do_setopt_accept_filter(struct socket *s
 	/*
 	 * Handle the simple delete case first.
 	 */
-	if (sopt == NULL) {
+	if (sopt == NULL || sopt->sopt_val == NULL) {
 		SOCK_LOCK(so);
 		if ((so->so_options & SO_ACCEPTCONN) == 0) {
 			SOCK_UNLOCK(so);
Index: tools/regression/sockets/accf_data_attach/accf_data_attach.c
===================================================================
RCS file: /home/ncvs/src/tools/regression/sockets/accf_data_attach/accf_data_attach.c,v
retrieving revision 1.3
diff -u -p -r1.3 accf_data_attach.c
--- tools/regression/sockets/accf_data_attach/accf_data_attach.c	11 Nov 2004 19:47:53 -0000	1.3
+++ tools/regression/sockets/accf_data_attach/accf_data_attach.c	11 Jun 2005 07:06:41 -0000
@@ -54,6 +54,8 @@
  *   state.
  * - That once an accept filter is attached, we can query to make sure it is
  *   attached.
+ * - That once an accept filter is attached, we can remove it and query to
+ *   make sure it is removed.
  */
 int
 main(int argc, char *argv[])
@@ -145,38 +147,72 @@ main(int argc, char *argv[])
 	printf("ok 7 - listen\n");

 	/*
-	 * Step 7: After listen().  This call to setsockopt() should succeed.
+	 * Step 7: Getsockopt() after listen().  Should fail with EINVAL,
+	 * since we have not installed accept filter yep.
+	 */
+	len = sizeof(afa);
+	ret = getsockopt(lso, SOL_SOCKET, SO_ACCEPTFILTER, &afa, &len);
+	if (ret == 0)
+		errx(-1, "not ok 8 - getsockopt() after listen() but before "
+		    "setsockopt() succeeded");
+	if (errno != EINVAL)
+		errx(-1, "not ok 8 - getsockopt() after listen() but before "
+		    "setsockopt() failed with %d (%s)", errno, strerror(errno));
+	printf("ok 8 - getsockopt\n");
+
+	/*
+	 * Step 8: After listen().  This call to setsockopt() should succeed.
 	 */
 	bzero(&afa, sizeof(afa));
 	strcpy(afa.af_name, ACCF_NAME);
 	ret = setsockopt(lso, SOL_SOCKET, SO_ACCEPTFILTER, &afa, sizeof(afa));
 	if (ret != 0)
-		errx(-1, "not ok 8 - setsockopt() after listen() failed with %d "
+		errx(-1, "not ok 9 - setsockopt() after listen() failed with %d "
 		    "(%s)", errno, strerror(errno));
 	if (len != sizeof(afa))
-		errx(-1, "not ok 8 - setsockopt() after listen() returned wrong "
+		errx(-1, "not ok 9 - setsockopt() after listen() returned wrong "
 		    "size (%d vs expected %d)", len, sizeof(afa));
-	printf("ok 8 - setsockopt\n");
+	printf("ok 9 - setsockopt\n");

 	/*
-	 * Step 8: After setsockopt().  Should succeed and identify
+	 * Step 9: After setsockopt().  Should succeed and identify
 	 * ACCF_NAME.
 	 */
 	bzero(&afa, sizeof(afa));
 	len = sizeof(afa);
 	ret = getsockopt(lso, SOL_SOCKET, SO_ACCEPTFILTER, &afa, &len);
 	if (ret != 0)
-		errx(-1, "not ok 9 - getsockopt() after listen() setsockopt() "
+		errx(-1, "not ok 10 - getsockopt() after listen() setsockopt() "
 		    "failed with %d (%s)", errno, strerror(errno));
 	if (len != sizeof(afa))
-		errx(-1, "not ok 9 - getsockopt() after setsockopet()  after "
+		errx(-1, "not ok 10 - getsockopt() after setsockopet()  after "
 		    "listen() returned wrong size (got %d expected %d)", len,
 		    sizeof(afa));
 	if (strcmp(afa.af_name, ACCF_NAME) != 0)
-		errx(-1, "not ok 9 - getsockopt() after setsockopt() after "
+		errx(-1, "not ok 10 - getsockopt() after setsockopt() after "
 		    "listen() mismatch (got %s expected %s)", afa.af_name,
 		    ACCF_NAME);
-	printf("ok 9 - getsockopt\n");
+	printf("ok 10 - getsockopt\n");
+
+	/*
+	 * Step 10: Remove accept filter.  After removing the accept filter
+	 * getsockopt() should fail with EINVAL.
+	 */
+	ret = setsockopt(lso, SOL_SOCKET, SO_ACCEPTFILTER, NULL, 0);
+	if (ret != 0)
+		errx(-1, "not ok 11 - setsockopt() after listen() "
+		    "failed with %d (%s)", errno, strerror(errno));
+	bzero(&afa, sizeof(afa));
+	len = sizeof(afa);
+	ret = getsockopt(lso, SOL_SOCKET, SO_ACCEPTFILTER, &afa, &len);
+	if (ret == 0)
+		errx(-1, "not ok 11 - getsockopt() after removing "
+		    "the accept filter returns valid accept filter %s",
+		    afa.af_name);
+	if (errno != EINVAL)
+		errx(-1, "not ok 11 - getsockopt() after removing the accept"
+		    "filter failed with %d (%s)", errno, strerror(errno));
+	printf("ok 11 - setsockopt\n");

 	close(lso);
 	return (0);
%%%

-- 
Maxim Konovalov