[Bug libc/3013] Uninitialized byte written in fa_IR/LC_CTYPE

["rsandifo at sourceware dot org" <sourceware-bugzilla at sourceware dot org>]

------- Additional Comments From rsandifo at sourceware dot org  2006-08-09 14:58 -------
FWIW, we've been using the first of the attached patches to fix this issue,
and to fix a couple of other problems we'd found in the localedef code.

There are two occurences of:

    iov[2 + elem + offset].iov_base = (void *) nulbytes;
    iov[2 + elem + offset].iov_len = 1 + (4 - ((total + 1) % 4));
    total += 1 + (4 - ((total + 1) % 4));

in ld-ctype.c, and like you say, it adds between 2 and 5 bytes.

This is a rather odd construct.  The data being added here is the nul
terminator for a list of strings.  I think the idea is to just to add
one nul and then align the following data to 4 bytes.  (The nul
terminator of the final string has already been added; the nul being
added here terminates the list itself.)  However, if we add a nul
terminator at file offset 4X + 3, the calculation:

   (4 - ((total + 1) % 4))

means that we'll insert 4 bytes of padding after the nul.  So what we
effectively have is two nul terminators followed by an alignment.

In theory it should be safe to remove the extra nul without affecting
readers of LC_CTYPE, but there might not be any appetite for changing
the format now.  I've therefore attached two patches.  The first patch
keeps the original padding and expands nulbytes[] accordingly.  The
second patch changes the padding calculation instead.

The other problems we'd found, and that are fixed by both patches, were:

- In the same function, the code to add DEFAULT_MISSING wasn't
  setting iov_len correctly:

       iov[2 + elem + offset].iov_base =
	 ctype->default_missing ?: (uint32_t *) L"";
       iov[2 + elem + offset].iov_len =
	 wcslen (iov[2 + elem + offset].iov_base);
       idx[elem + 1] = idx[elem] + iov[2 + elem + offset].iov_len;

  wcslen() returns a character count, not a byte count, so the iov_len
  value must be multiplied by sizeof (uint32_t).  This showed up for a
  single-character DEFAULT_MISSING on a big-endian target.

- The same function builds up class_b[] as follows:

     for (nr = 0; nr < ctype->nr_charclass; nr++)
       {
	 ctype->class_b[nr] = (uint32_t *) xcalloc (256 / 32, sizeof (uint32_t));

	 for (idx = 0; idx < 256; ++idx)
	   if (ctype->class256_collection[idx] & _ISbit (nr))
	     ctype->class_b[nr][idx >> 5] |= (uint32_t)1 << (idx & 0x1f);
       }

  The problem here is that _ISbit() is only designed to handle
  arguments in the range [0,11] and nr_charclass can be higher
  than that.   The code deals with this correctly when setting
  class256_collection -- for example:

	 class256_bit = cnt <= 11 ? _ISbit (cnt) : 0;

  -- but not when reading it.

  This causes problems for little-endian machines, for which
  _ISbit() is defined as follows:

     #define _ISbit(bit) \
       ((bit) < 8 ? ((1 << (bit)) << 8) : ((1 << (bit)) >> 8))

  nr_charclass == 19 (IIRC) for the Japenese files, and this definition
  of _ISbit() treats bits [16,18] like bits [0,2].

- time_output was failing to align _NL_W_DATE_FMT.

Both patches tested on x86_64-linux-gnu.

Richard


-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Uninitialized byte written  |Uninitialized byte written
                   |in fa_IR/LC_CTYPE           |in fa_IR/LC_CTYPE


http://sourceware.org/bugzilla/show_bug.cgi?id=3013

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.