NetBSD Problem Report #47591

From www@NetBSD.org  Mon Feb 25 06:27:36 2013
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	by www.NetBSD.org (Postfix) with ESMTP id C7C6C63E522
	for <gnats-bugs@gnats.NetBSD.org>; Mon, 25 Feb 2013 06:27:35 +0000 (UTC)
Message-Id: <20130225062734.5779C63E522@www.NetBSD.org>
Date: Mon, 25 Feb 2013 06:27:34 +0000 (UTC)
From: akr@fsij.org
Reply-To: akr@fsij.org
To: gnats-bugs@NetBSD.org
Subject: Unix domain accept() returns the server socket name if client socket is closed before accept() call.
X-Send-Pr-Version: www-1.0

>Number:         47591
>Category:       kern
>Synopsis:       Unix domain accept() returns the server socket name if client socket is closed before accept() call.
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Feb 25 06:30:00 +0000 2013
>Closed-Date:    Fri Dec 20 12:01:11 +0000 2013
>Last-Modified:  Fri Dec 20 12:01:11 +0000 2013
>Originator:     Tanaka Akira
>Release:        NetBSD 6.0.1
>Organization:
>Environment:
NetBSD netbsd6 6.0.1 NetBSD 6.0.1 (GENERIC) amd64
>Description:
I found that accept() for Unix domain socket can return curious socket names.

The test program creates a Unix domain server socket named as "socket-file"
and a client socket connected to that.  The program shows the address obtained
by accept().

However the client socket is closed before accept() call for the server
socket.

In that case, accept() returns the server socket name with garbage.

I think this behavior is a bug because accept() should return client socket name.

The result of test program is follows.

  % ./a.out 
  client socket length: 106
  client socket name: j\x01socket-file\x00\x00\x00
  \x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00
  \x00,9>\x80\xfe\xff\xff\x00\x00\x00\x00\x00\x00\x00\x00
  \x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00
  \x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00
  \x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00
  \x00\x00\x00\x00\x00\x00\x00\x00\x00\x00
  % rm socket-file; ./a.out
  client socket length: 106
  client socket name: j\x01socket-file\x00\x00\x00
  \x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00
  \x00\xac\xd4>\x80\xfe\xff\xff\x00\x00\x00\x00\x00\x00\x00\x00
  \x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00
  \x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00
  \x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00
  \x00\x00\x00\x00\x00\x00\x00\x00\x00\x00

The "client socket name" field shows struct sockaddr_un.
The first 2 bytes, "j\x01", is sun_len and sun_family.
sun_path field placed after that begins with the server socket name,
"socket-file".
sun_path field also contains garbage which is changed slightly
for each run.


>How-To-Repeat:
% uname -a
NetBSD netbsd6 6.0.1 NetBSD 6.0.1 (GENERIC) amd64
% ls
tst.c
% cat tst.c         
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>
#include <sys/socket.h>
#include <sys/un.h>

int main(int argc, char *argv[])
{
  int serv, clnt, acpt;
  struct sockaddr_un addr;
  struct sockaddr_un acpt_addr;
  socklen_t acept_addrlen;
  int ret;
  int i;

  serv = socket(AF_UNIX, SOCK_STREAM, 0);
  if (serv == -1) { perror("socket"); exit(EXIT_FAILURE); }

  memset(&addr, '\0', sizeof(addr));
  addr.sun_family = AF_UNIX;
  strcpy(addr.sun_path, "socket-file");

  ret = bind(serv, (struct sockaddr *)&addr, sizeof(addr));
  if (ret == -1) { perror("bind"); exit(EXIT_FAILURE); }

  ret = listen(serv, SOMAXCONN);
  if (ret == -1) { perror("listen"); exit(EXIT_FAILURE); }

  clnt = socket(AF_UNIX, SOCK_STREAM, 0);
  if (clnt == -1) { perror("socket"); exit(EXIT_FAILURE); }

  ret = connect(clnt, (struct sockaddr *)&addr, sizeof(addr));
  if (ret == -1) { perror("connect"); exit(EXIT_FAILURE); }

  ret = close(clnt);
  if (ret == -1) { perror("close"); exit(EXIT_FAILURE); }

  acept_addrlen = sizeof(acpt_addr);
  acpt = accept(serv, (struct sockaddr *)&acpt_addr, &acept_addrlen);
  if (acpt == -1) { perror("accept"); exit(EXIT_FAILURE); }

  printf("client socket length: %d\n", (int)acept_addrlen);
  printf("client socket name: ");
  for (i = 0; i < acept_addrlen; i++) {
    int ch = ((unsigned char *)&acpt_addr)[i];
    if (ch < ' ' || '~' < ch)
      printf("\\x%02x", ch);
    else
      printf("%c", ch);
    if (i % 16 == 15)
      printf("\n");
  }
  printf("\n");

  return EXIT_SUCCESS;
}
% gcc -Wall -g tst.c
% ./a.out 
client socket length: 106
client socket name: j\x01socket-file\x00\x00\x00
\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00
\x00,9>\x80\xfe\xff\xff\x00\x00\x00\x00\x00\x00\x00\x00
\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00
\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00
\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00
\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00
% rm socket-file; ./a.out
client socket length: 106
client socket name: j\x01socket-file\x00\x00\x00
\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00
\x00\xac\xd4>\x80\xfe\xff\xff\x00\x00\x00\x00\x00\x00\x00\x00
\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00
\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00
\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00
\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00
% rm socket-file; ./a.out
client socket length: 106
client socket name: j\x01socket-file\x00\x00\x00
\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00
\x00d\xcd9\x80\xfe\xff\xff\x00\x00\x00\x00\x00\x00\x00\x00
\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00
\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00
\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00
\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00
% 

>Fix:

>Release-Note:

>Audit-Trail:
From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/47591 CVS commit: src/sys/kern
Date: Mon, 7 Oct 2013 20:29:24 -0400

 Module Name:	src
 Committed By:	christos
 Date:		Tue Oct  8 00:29:24 UTC 2013

 Modified Files:
 	src/sys/kern: uipc_syscalls.c

 Log Message:
 PR/47591: Michael Plass: If the unix socket is closed before accept,
 unp->unp_conn will be NULL in PRU_ACCEPT, as called from
 sys_accept->so_accept. This will cause the usrreq to return with
 no error, leaving the mbuf gotten from m_get() with an uninitialized
 length, containing junk from a previous call. Initialize m_len to
 be 0 to handle this case. This is yet another reason why Beverly's
 idea of setting m_len = 0 in m_get() makes a lot of sense. Arguably
 this could be an error, since the data we return now has 0 family
 and length.


 To generate a diff of this commit:
 cvs rdiff -u -r1.162 -r1.163 src/sys/kern/uipc_syscalls.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Stephen Borrill" <sborrill@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/47591 CVS commit: [netbsd-5] src/sys/kern
Date: Fri, 13 Dec 2013 12:22:39 +0000

 Module Name:	src
 Committed By:	sborrill
 Date:		Fri Dec 13 12:22:39 UTC 2013

 Modified Files:
 	src/sys/kern [netbsd-5]: uipc_syscalls.c

 Log Message:
 Pull up the following revisions(s) (requested by spz in ticket #1891):
 	sys/kern/uipc_syscalls.c:	revision 1.163

 If the unix socket is closed before accept, the mbuf returned by
 m_get() will have an uninitialized length and contain junk from a
 previous call. Initialize m_len to be 0 to handle this case.
 Fixes PR/47591


 To generate a diff of this commit:
 cvs rdiff -u -r1.134.4.3 -r1.134.4.4 src/sys/kern/uipc_syscalls.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Manuel Bouyer" <bouyer@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/47591 CVS commit: [netbsd-6] src/sys/kern
Date: Sat, 14 Dec 2013 19:36:33 +0000

 Module Name:	src
 Committed By:	bouyer
 Date:		Sat Dec 14 19:36:33 UTC 2013

 Modified Files:
 	src/sys/kern [netbsd-6]: uipc_syscalls.c

 Log Message:
 Pull up following revision(s) (requested by spz in ticket #996):
 	sys/kern/uipc_syscalls.c: revision 1.163
 PR/47591: Michael Plass: If the unix socket is closed before accept,
 unp->unp_conn will be NULL in PRU_ACCEPT, as called from
 sys_accept->so_accept. This will cause the usrreq to return with
 no error, leaving the mbuf gotten from m_get() with an uninitialized
 length, containing junk from a previous call. Initialize m_len to
 be 0 to handle this case. This is yet another reason why Beverly's
 idea of setting m_len = 0 in m_get() makes a lot of sense. Arguably
 this could be an error, since the data we return now has 0 family
 and length.


 To generate a diff of this commit:
 cvs rdiff -u -r1.154.2.4 -r1.154.2.5 src/sys/kern/uipc_syscalls.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Manuel Bouyer" <bouyer@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/47591 CVS commit: [netbsd-6-1] src/sys/kern
Date: Sat, 14 Dec 2013 19:36:58 +0000

 Module Name:	src
 Committed By:	bouyer
 Date:		Sat Dec 14 19:36:58 UTC 2013

 Modified Files:
 	src/sys/kern [netbsd-6-1]: uipc_syscalls.c

 Log Message:
 Pull up following revision(s) (requested by spz in ticket #996):
 	sys/kern/uipc_syscalls.c: revision 1.163
 PR/47591: Michael Plass: If the unix socket is closed before accept,
 unp->unp_conn will be NULL in PRU_ACCEPT, as called from
 sys_accept->so_accept. This will cause the usrreq to return with
 no error, leaving the mbuf gotten from m_get() with an uninitialized
 length, containing junk from a previous call. Initialize m_len to
 be 0 to handle this case. This is yet another reason why Beverly's
 idea of setting m_len = 0 in m_get() makes a lot of sense. Arguably
 this could be an error, since the data we return now has 0 family
 and length.


 To generate a diff of this commit:
 cvs rdiff -u -r1.154.2.4 -r1.154.2.4.2.1 src/sys/kern/uipc_syscalls.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Manuel Bouyer" <bouyer@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/47591 CVS commit: [netbsd-6-0] src/sys/kern
Date: Sat, 14 Dec 2013 19:37:02 +0000

 Module Name:	src
 Committed By:	bouyer
 Date:		Sat Dec 14 19:37:02 UTC 2013

 Modified Files:
 	src/sys/kern [netbsd-6-0]: uipc_syscalls.c

 Log Message:
 Pull up following revision(s) (requested by spz in ticket #996):
 	sys/kern/uipc_syscalls.c: revision 1.163
 PR/47591: Michael Plass: If the unix socket is closed before accept,
 unp->unp_conn will be NULL in PRU_ACCEPT, as called from
 sys_accept->so_accept. This will cause the usrreq to return with
 no error, leaving the mbuf gotten from m_get() with an uninitialized
 length, containing junk from a previous call. Initialize m_len to
 be 0 to handle this case. This is yet another reason why Beverly's
 idea of setting m_len = 0 in m_get() makes a lot of sense. Arguably
 this could be an error, since the data we return now has 0 family
 and length.


 To generate a diff of this commit:
 cvs rdiff -u -r1.154.2.1.4.1 -r1.154.2.1.4.2 src/sys/kern/uipc_syscalls.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->feedback
State-Changed-By: bouyer@NetBSD.org
State-Changed-When: Sat, 14 Dec 2013 19:44:46 +0000
State-Changed-Why:
Hello,
a fix has been commioted to HEAD and pulled up to all netbsd-6 branches.
Is it OK to close the PR ?


From: Tanaka Akira <akr@fsij.org>
To: gnats-bugs@netbsd.org
Cc: kern-bug-people@netbsd.org, netbsd-bugs@netbsd.org, gnats-admin@netbsd.org, 
	bouyer@netbsd.org
Subject: Re: kern/47591 (Unix domain accept() returns the server socket name
 if client socket is closed before accept() call.)
Date: Fri, 20 Dec 2013 17:54:20 +0900

 2013/12/15  <bouyer@netbsd.org>:
 > Synopsis: Unix domain accept() returns the server socket name if client socket is closed before accept() call.

 > Hello,
 > a fix has been commioted to HEAD and pulled up to all netbsd-6 branches.
 > Is it OK to close the PR ?

 Thank you.
 I confirmed that the problem is fixed in NetBSD 6.99.28.

 No problem to close this PR.

 $ uname -mrsv
 NetBSD 6.99.28 NetBSD 6.99.28 (GENERIC) #0: Tue Dec 17 11:48:16 UTC
 2013  builds@b8.netbsd.org:/home/builds/ab/HEAD/amd64/201312171040Z-obj/home/builds/ab/HEAD/src/sys/arch/amd64/compile/GENERIC
 amd64
 -- 
 Tanaka Akira

State-Changed-From-To: feedback->closed
State-Changed-By: bouyer@NetBSD.org
State-Changed-When: Fri, 20 Dec 2013 12:01:11 +0000
State-Changed-Why:
Confirmed fixed.


>Unformatted:

Home	PR Database Search