Bug#63373: marked as done (suspicious code in isofs)

[Debian Bug Tracking System]

Your message dated Sun, 27 May 2001 02:17:34 +0200
with message-id <20010527021734.A577@212.23.136.22>
and subject line I fixed that! I fixed that!
has caused the attached Bug report to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
Bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what I am
talking about this indicates a serious mail system misconfiguration
somewhere.  Please contact me immediately.)

Darren Benham
(administrator, Debian Bugs database)

--------------------------------------
Received: (at submit) by bugs.debian.org; 1 May 2000 15:10:35 +0000
Received: (qmail 12640 invoked from network); 1 May 2000 15:10:34 -0000
Received: from sunu450.rz.ruhr-uni-bochum.de (134.147.64.5)
  by master.debian.org with SMTP; 1 May 2000 15:10:34 -0000
Received: (qmail 7369 invoked from network); 1 May 2000 15:10:26 -0000
Received: from dialppp-1-133.rz.ruhr-uni-bochum.de (HELO localhost) 
(mail@134.147.1.133)
  by mailhost.rz.ruhr-uni-bochum.de with SMTP; 1 May 2000 15:10:26 -0000
Received: from marcus by localhost with local (Exim 3.12 #1 (Debian))
        for submit@bugs.debian.org
        id 12mHjY-0003rk-00; Mon, 01 May 2000 17:03:40 +0200
Date: Mon, 1 May 2000 17:03:40 +0200
From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: submit@bugs.debian.org
Subject: suspicious code in isofs
Message-ID: <20000501170340.A14823@ulysses.dhis.net>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
User-Agent: Mutt/1.1.4i
Organization: Marcus Brinkmann's Home

Package: hurd
Version: N/A

Hi,

I hope I don't make fuss about nothing, in case I don't understand the code.
Anyway, it looks strange.

In isofs/lookup.c (diskfs_get_directs), there is the first for-loop, which
seems to check if there are enough entries available.

I want to draw your attention to the "Ignore and skip RE entries" comment.

  /* Skip to ENTRY */
  dirbuf = disk_image + (dp->dn->file_start << store->log2_block_size);
  bufp = dirbuf;
  for (i = 0; i < entry; i ++)
    {
      struct rrip_lookup rr;

      ep = (struct dirrect *) bufp;
      rrip_lookup (ep, &rr, 0);

      /* Ignore and skip RE entries */
      if (rr.valid & VALID_RE)
        {
          bufp = bufp + ep->len;
          release_rrip (&rr);
          continue;
        }

     ....
   }

It seems as if those RE entries should not be counted. This is supported by
the following while-loop, which copies the valid entries into the return
buffer. However, this can't work! because a continue in the for-loop will
nevertheless increment the loop variable i. It looks as if the code was
cut&pasted from the while loop, or an earlier while loop was changed into a
for loop or so.

If I had to guess, I would add a i-- in the if-block.

But this is not all. The code looks suspicious in a second way, too.
The value the incremented bufp points to is not checked for null.
I don't know the ISO9660 standard, but if it could be that a RE entry is at
the end of a logical sector, this code will overrun, because the validity
check at the end of the for-loop is skipped (because of the continue):

      ...

      bufp = bufp + ep->len;

      /* If BUFP points at a null, then we have hit the last
         record in this logical sector.  In that case, skip up to
         the next logical sector. */
      if (*(char *)bufp == '\0')
        bufp = (void *) (((long) bufp & ~(logical_sector_size - 1))
                         + logical_sector_size);
    }

If my analysis is true, the code needs to be rearranged a bit.

Thanks,
Marcus

-- 
`Rhubarb is no Egyptian god.' Debian http://www.debian.org Check Key server 
Marcus Brinkmann              GNU    http://www.gnu.org    for public PGP Key 
Marcus.Brinkmann@ruhr-uni-bochum.de,     marcus@gnu.org    PGP Key ID 36E7CD09
http://homepage.ruhr-uni-bochum.de/Marcus.Brinkmann/       brinkmd@debian.org
---------------------------------------
Received: (at 63373-close) by bugs.debian.org; 27 May 2001 00:17:38 +0000
>From Marcus.Brinkmann@ruhr-uni-bochum.de Sat May 26 19:17:38 2001
Return-path: <Marcus.Brinkmann@ruhr-uni-bochum.de>
Received: from (localhost) [212.23.136.22] (mail)
        by master.debian.org with esmtp (Exim 3.12 1 (Debian))
        id 153oFV-0002Fb-00; Sat, 26 May 2001 19:17:38 -0500
Received: from marcus by localhost with local (Exim 3.22 #1 (Debian))
        id 153oFT-0002W5-00
        for <63373-close@bugs.debian.org>; Sun, 27 May 2001 02:17:35 +0200
Date: Sun, 27 May 2001 02:17:34 +0200
From: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
To: 63373-close@bugs.debian.org
Subject: I fixed that! I fixed that!
Message-ID: <20010527021734.A577@212.23.136.22>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
User-Agent: Mutt/1.3.15i
Sender: Marcus Brinkmann <Marcus.Brinkmann@ruhr-uni-bochum.de>
Delivered-To: 63373-close@bugs.debian.org

fixed in hurd 20010426

Marcus

-- 
`Rhubarb is no Egyptian god.' Debian http://www.debian.org brinkmd@debian.org
Marcus Brinkmann              GNU    http://www.gnu.org    marcus@gnu.org
Marcus.Brinkmann@ruhr-uni-bochum.de
http://www.marcus-brinkmann.de