bug#8277: Emacs should use socklen_t for socket lengths

bug#8277: Emacs should use socklen_t for socket lengths

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 4647 bytes --]

In several places in the Emacs trunk src/process.c, the type
'int' is used where POSIX says socklen_t should be used.
The two types are typically the same, or at least the same
size, but on some platforms (e.g., 64-bit HP-UX) they
have different sizes and pointers to them can't be safely
interchanged.

I plan to install the following patch, which uses the gnulib
socklen module to provide a definition of socklen_t
on platforms that do not already define it, and then
substitutes 'socklen_t' for the relevant occurrences of 'int' in
src/process.c.  MS-DOS and MS-Windows ports may be affected by
this, since it adds an "#undef socklen_t" to src/config.in.

The patch below contains just the hand-maintained source files;
the full patch (including autogenerated files) is attached.

=== modified file 'ChangeLog'
--- ChangeLog	2011-03-13 17:39:04 +0000
+++ ChangeLog	2011-03-18 03:30:24 +0000
@@ -1,3 +1,9 @@
+2011-03-17  Paul Eggert  <eggert@cs.ucla.edu>
+
+	* Makefile.in (GNULIB_MODULES): Add socklen.
+	* configure.in: Do not check for sys/socket.h, since socklen does that.
+	* m4/socklen.m4: New automatically-generated file, from gnulib.
+
 2011-03-13  Paul Eggert  <eggert@cs.ucla.edu>
 
 	Update for gnulib.

=== modified file 'Makefile.in'
--- Makefile.in	2011-03-13 17:39:04 +0000
+++ Makefile.in	2011-03-18 03:30:24 +0000
@@ -332,7 +332,7 @@
 # as per $(gnulib_srcdir)/DEPENDENCIES.
 GNULIB_MODULES = \
   crypto/md5 dtoastr filemode getloadavg getopt-gnu \
-  ignore-value intprops lstat mktime readlink strftime symlink sys_stat
+  ignore-value intprops lstat mktime readlink socklen strftime symlink sys_stat
 GNULIB_TOOL_FLAGS = \
  --import --no-changelog --no-vc-files --makefile-name=gnulib.mk
 sync-from-gnulib: $(gnulib_srcdir)

=== modified file 'configure.in'
--- configure.in	2011-03-12 19:19:47 +0000
+++ configure.in	2011-03-18 03:30:24 +0000
@@ -1265,7 +1265,6 @@
   AC_DEFINE(NO_MATHERR, 1, [Define to 1 if you don't have struct exception in math.h.])
 fi
 
-AC_CHECK_HEADERS(sys/socket.h)
 AC_CHECK_HEADERS(net/if.h, , , [AC_INCLUDES_DEFAULT
 #if HAVE_SYS_SOCKET_H
 #include <sys/socket.h>

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2011-03-17 16:51:42 +0000
+++ src/ChangeLog	2011-03-18 03:30:24 +0000
@@ -1,3 +1,13 @@
+2011-03-18  Paul Eggert  <eggert@cs.ucla.edu>
+
+	* process.c (Fmake_network_process): Use socklen_t, not int,
+	where POSIX says socklen_t is required in portable programs.
+	This fixes a porting bug on hosts like 64-bit HP-UX, where
+	socklen_t is wider than int.
+	(Fmake_network_process, server_accept_connection):
+	(wait_reading_process_output, read_process_output):
+	Likewise.
+
 2011-03-17  Paul Eggert  <eggert@cs.ucla.edu>
 
 	Fix more problems found by GCC 4.5.2's static checks.

=== modified file 'src/process.c'
--- src/process.c	2011-03-17 05:18:33 +0000
+++ src/process.c	2011-03-18 03:30:24 +0000
@@ -3467,7 +3467,7 @@
 	  if (EQ (service, Qt))
 	    {
 	      struct sockaddr_in sa1;
-	      int len1 = sizeof (sa1);
+	      socklen_t len1 = sizeof (sa1);
 	      if (getsockname (s, (struct sockaddr *)&sa1, &len1) == 0)
 		{
 		  ((struct sockaddr_in *)(lres->ai_addr))->sin_port = sa1.sin_port;
@@ -3514,7 +3514,8 @@
 	  /* Unlike most other syscalls connect() cannot be called
 	     again.  (That would return EALREADY.)  The proper way to
 	     wait for completion is select(). */
-	  int sc, len;
+	  int sc;
+	  socklen_t len;
 	  SELECT_TYPE fdset;
 	retry_select:
 	  FD_ZERO (&fdset);
@@ -3587,7 +3588,7 @@
       if (!is_server)
 	{
 	  struct sockaddr_in sa1;
-	  int len1 = sizeof (sa1);
+	  socklen_t len1 = sizeof (sa1);
 	  if (getsockname (s, (struct sockaddr *)&sa1, &len1) == 0)
 	    contact = Fplist_put (contact, QClocal,
 				  conv_sockaddr_to_lisp ((struct sockaddr *)&sa1, len1));
@@ -4192,7 +4193,7 @@
     struct sockaddr_un un;
 #endif
   } saddr;
-  int len = sizeof saddr;
+  socklen_t len = sizeof saddr;
 
   s = accept (channel, &saddr.sa, &len);
 
@@ -5059,7 +5060,7 @@
 	      /* getsockopt(,,SO_ERROR,,) is said to hang on some systems.
 		 So only use it on systems where it is known to work.  */
 	      {
-		int xlen = sizeof (xerrno);
+		socklen_t xlen = sizeof (xerrno);
 		if (getsockopt (channel, SOL_SOCKET, SO_ERROR, &xerrno, &xlen))
 		  xerrno = errno;
 	      }
@@ -5171,7 +5172,7 @@
   /* We have a working select, so proc_buffered_char is always -1.  */
   if (DATAGRAM_CHAN_P (channel))
     {
-      int len = datagram_address[channel].len;
+      socklen_t len = datagram_address[channel].len;
       nbytes = recvfrom (channel, chars + carryover, readmax,
 			 0, datagram_address[channel].sa, &len);
     }



[-- Attachment #2: patch.txt.gz --]
[-- Type: application/x-gzip, Size: 4537 bytes --]

bug#8277: Emacs should use socklen_t for socket lengths

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 934 bytes --]

On 03/17/2011 09:40 PM, Paul Eggert wrote:

> I plan to install the following patch, which uses the gnulib
> socklen module to provide a definition of socklen_t
> on platforms that do not already define it, and then
> substitutes 'socklen_t' for the relevant occurrences of 'int' in
> src/process.c.  MS-DOS and MS-Windows ports may be affected by
> this, since it adds an "#undef socklen_t" to src/config.in.

Comments by Bruno Haible on the gnulib mailing list
<http://lists.gnu.org/archive/html/bug-gnulib/2011-03/msg00211.html>
showed the need for an update to that patch, for the
benefit of Cygwin and MingW ports.  I've attached it; it
consists entirely of autogenerated files from Emacs's point
of view.  This adds a symbol HAVE_WS2TCPIP_H to src/config.in,
which may need to be configured for MS-DOS and MS-Windows.
I haven't committed any of this socklen_t stuff to the trunk yet,
but plan to do so after a bit more testing.

[-- Attachment #2: patch.txt.gz --]
[-- Type: application/x-gzip, Size: 1948 bytes --]

bug#8277: Emacs should use socklen_t for socket lengths

[Eli Zaretskii]

> Date: Sun, 20 Mar 2011 09:38:48 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: Eli Zaretskii <eliz@gnu.org>
> 
> Comments by Bruno Haible on the gnulib mailing list
> <http://lists.gnu.org/archive/html/bug-gnulib/2011-03/msg00211.html>
> showed the need for an update to that patch, for the
> benefit of Cygwin and MingW ports.  I've attached it; it
> consists entirely of autogenerated files from Emacs's point
> of view.  This adds a symbol HAVE_WS2TCPIP_H to src/config.in,
> which may need to be configured for MS-DOS and MS-Windows.
> I haven't committed any of this socklen_t stuff to the trunk yet,
> but plan to do so after a bit more testing.

I don't understand Bruno's comments.  He probably thinks that the
Windows build uses the Posix configury, otherwise he would have
realized that, with the single exception of process.c, none of the
patched files is used in the Windows build.

So it looks like all we need to do for the native Windows build is
define socket_t and that's it.  Or did I miss something?

As for the MS-DOS build of Emacs, it does not support networking, and
in fact does not even compile the parts of process.c that are being
changed (it has `subprocesses' undefined).  So this is surely
irrelevant for that port.

I don't know enough about Cygwin to provide any feedback on this.

bug#8277: Emacs should use socklen_t for socket lengths

[Paul Eggert]

On 03/20/2011 11:08 AM, Eli Zaretskii wrote:
> I don't understand Bruno's comments.  He probably thinks that the
> Windows build uses the Posix configury

My understanding of his comments is that he was thinking about
building Emacs under Cygwin and/or Mingw, and that it's those ports
these fixes are designed for, not the native MS-Windows port.

> So it looks like all we need to do for the native Windows build is
> define socket_t and that's it.  Or did I miss something?

Not as far as I know.

bug#8277: Emacs should use socklen_t for socket lengths

[Eli Zaretskii]

> Date: Sun, 20 Mar 2011 11:34:48 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: bug-gnu-emacs@gnu.org
> 
> On 03/20/2011 11:08 AM, Eli Zaretskii wrote:
> > I don't understand Bruno's comments.  He probably thinks that the
> > Windows build uses the Posix configury
> 
> My understanding of his comments is that he was thinking about
> building Emacs under Cygwin and/or Mingw, and that it's those ports
> these fixes are designed for, not the native MS-Windows port.

The MinGW build and the native MS-Windows port of Emacs are one and
the same, there's no other MinGW build of Emacs.  (Emacs can also be
built with the Microsoft compiler, but that's almost unsupported.)

bug#8277: Emacs should use socklen_t for socket lengths

[Paul Eggert]

On 03/20/2011 11:45 AM, Eli Zaretskii wrote:
>> My understanding of his comments is that he was thinking about
>> > building Emacs under Cygwin and/or Mingw, and that it's those ports
>> > these fixes are designed for, not the native MS-Windows port.
> The MinGW build and the native MS-Windows port of Emacs are one and
> the same, there's no other MinGW build of Emacs.

Thanks, I didn't know that.  In that case I expect his comments
were aimed more at the Cygwin port.

bug#8277: Emacs should use socklen_t for socket lengths

[Eli Zaretskii]

> Date: Sun, 20 Mar 2011 11:53:58 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: bug-gnu-emacs@gnu.org
> 
> On 03/20/2011 11:45 AM, Eli Zaretskii wrote:
> >> My understanding of his comments is that he was thinking about
> >> > building Emacs under Cygwin and/or Mingw, and that it's those ports
> >> > these fixes are designed for, not the native MS-Windows port.
> > The MinGW build and the native MS-Windows port of Emacs are one and
> > the same, there's no other MinGW build of Emacs.
> 
> Thanks, I didn't know that.  In that case I expect his comments
> were aimed more at the Cygwin port.

But for Cygwin, nt/inc/sys/socket.h is not relevant.  Files under nt/
are used only by the native Windows build.  I expect Cygwin to use
sys/socket.h from its system headers, and I don't think it includes
ws2tcpip.h (but I could be wrong).

bug#8277: Emacs should use socklen_t for socket lengths

[Paul Eggert]

On 03/20/2011 12:22 PM, Eli Zaretskii wrote:
> But for Cygwin, nt/inc/sys/socket.h is not relevant.  Files under nt/
> are used only by the native Windows build.  I expect Cygwin to use
> sys/socket.h from its system headers, and I don't think it includes
> ws2tcpip.h (but I could be wrong).

In that case, I suppose Bruno was talking about ports for programs
other than Emacs.  Cygwin ports should include sys/socket.h, as
you wrote, and MingW ports should include some other file.  For
Emacs this shouldn't matter, since it does things its own way,
and (and you say) all that's important is that the header files
should define socklen_t one way or another.

bug#8277: Emacs should use socklen_t for socket lengths

[Ken Brown]

On 3/20/2011 3:22 PM, Eli Zaretskii wrote:
>> Date: Sun, 20 Mar 2011 11:53:58 -0700
>> From: Paul Eggert<eggert@cs.ucla.edu>
>> CC: bug-gnu-emacs@gnu.org
>>
>> On 03/20/2011 11:45 AM, Eli Zaretskii wrote:
>>>> My understanding of his comments is that he was thinking about
>>>>> building Emacs under Cygwin and/or Mingw, and that it's those ports
>>>>> these fixes are designed for, not the native MS-Windows port.
>>> The MinGW build and the native MS-Windows port of Emacs are one and
>>> the same, there's no other MinGW build of Emacs.
>>
>> Thanks, I didn't know that.  In that case I expect his comments
>> were aimed more at the Cygwin port.
>
> But for Cygwin, nt/inc/sys/socket.h is not relevant.  Files under nt/
> are used only by the native Windows build.  I expect Cygwin to use
> sys/socket.h from its system headers, and I don't think it includes
> ws2tcpip.h (but I could be wrong).

You're right.