Bug#389084: libc6: nasty bug in xdrmem_setpos(): signed comparison op is used for the pointers

[Nick Orlov <bugfixer@list.ru>]

Package: libc6
Version: 2.3.6.ds1-4
Severity: normal
Tags: patch


We've found a nasty bug in xdrmem_setpos() function.
Issue is caused by casts (char*) -> (long) and signed comparison
afterwards.  Which causes xdrmem_setpos() to fail completely unexpectedly
if mem buffer starts little bit below 2Gb point and ends little bit above
(that's when the sign of 32bit int is flipping).

Proposed patch is somewhat trivial:


--- xdr_mem.c.orig	2002-12-16 05:25:27.000000000 -0500
+++ xdr_mem.c	2006-09-23 13:39:01.000000000 -0400
@@ -171,19 +171,21 @@
  * xdrs modified
  */
 static bool_t
-xdrmem_setpos (xdrs, pos)
-     XDR *xdrs;
-     u_int pos;
+xdrmem_setpos (XDR *xdrs, u_int pos)
 {
   caddr_t newaddr = xdrs->x_base + pos;
   caddr_t lastaddr = xdrs->x_private + xdrs->x_handy;
+  size_t dist = 0;
 
-  if ((long) newaddr > (long) lastaddr
-      || (UINT_MAX < LONG_MAX
-	  && (long) UINT_MAX < (long) lastaddr - (long) newaddr))
+  if ( newaddr > lastaddr )
     return FALSE;
+
+  dist = lastaddr - newaddr;
+  if ( (u_int)dist != dist )
+    return FALSE;
+
   xdrs->x_private = newaddr;
-  xdrs->x_handy = (long) lastaddr - (long) newaddr;
+  xdrs->x_handy = (u_int)dist;
   return TRUE;
 }
 
___

I have not recompiled the whole libc locally, but just this file and
I've compared disasm dumps of original and patched version of the
function:


--- disass.orig	2006-09-23 13:54:10.000000000 -0400
+++ disass.patched	2006-09-23 13:53:20.000000000 -0400
@@ -12,7 +12,7 @@
 0x00000075 <xdrmem_setpos+21>:  add    %esi,%eax
 0x00000077 <xdrmem_setpos+23>:  xor    %esi,%esi
 0x00000079 <xdrmem_setpos+25>:  cmp    %eax,%edx
-0x0000007b <xdrmem_setpos+27>:  jg     0x8a <xdrmem_setpos+42>
+0x0000007b <xdrmem_setpos+27>:  ja     0x8a <xdrmem_setpos+42>
 0x0000007d <xdrmem_setpos+29>:  sub    %edx,%eax
 0x0000007f <xdrmem_setpos+31>:  mov    $0x1,%esi
 0x00000084 <xdrmem_setpos+36>:  mov    %edx,0xc(%ecx)

As you can see the only result of the changes is the type of cmp op:
signed (jg) -> unsigned (ja)

We believe that all the distros are affected by this (file has not been
touched for a three years or so), so probably patch have to be pushed
upstream asap. In particular this hurts our production (various versions
of RHEL) and hurts a lot...

100% of the credit for finding this goes to Konstantine Smaguine.
I've just prepared the patch and sent report to (hopefully) right people.

Thank you,
	Nick Orlov.

-- System Information:
Debian Release: testing/unstable
  APT prefers unstable
  APT policy: (500, 'unstable')
Architecture: i386 (i686)
Shell:  /bin/sh linked to /bin/bash
Kernel: Linux 2.6.18-rc6-mm1-8.swp
Locale: LANG=ru_RU.KOI8-R, LC_CTYPE=ru_RU.KOI8-R (charmap=KOI8-R)

Versions of packages libc6 depends on:
ii  tzdata                        2006l-1    Time Zone and Daylight Saving Time

libc6 recommends no packages.

-- debconf-show failed