NetBSD Problem Report #19852

Received: (qmail 21876 invoked by uid 605); 14 Jan 2003 22:54:34 -0000
Message-Id: <20030114235429.736572f9.christianbiere@gmx.de>
Date: Tue, 14 Jan 2003 23:54:29 +0100
From: Christian Biere <christianbiere@gmx.de>
Sender: gnats-bugs-owner@netbsd.org
To: gnats-bugs@gnats.netbsd.org
Subject: Potential problem with dump and large amount of memory

>Number:         19852
>Category:       bin
>Synopsis:       Potential problem with dump and more than 2GB of memory
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    bin-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Jan 14 22:55:00 +0000 2003
>Closed-Date:    Wed Jan 27 13:46:43 +0000 2010
>Last-Modified:  Wed Jan 27 13:47:55 +0000 2010
>Originator:     Christian Biere
>Release:        NetBSD 1.6K
>Organization:
>Environment:

>Description:

/usr/src/sbin/dump/rcache.c:

static int cachebufs;

[...]

void
initcache(int cachesize, int readblksize)
{
    size_t len;
    size_t  sharedSize;

    nblksread = (readblksize + ufsib->ufs_bsize - 1) / ufsib->ufs_bsize;
    if(cachesize == -1) {   /* Compute from memory available */
        int usermem;
        int mib[2] = { CTL_HW, HW_USERMEM };

        len = sizeof(usermem);
        if (sysctl(mib, 2, &usermem, &len, NULL, 0) < 0) {
            msg("sysctl(hw.usermem) failed: %s\n", strerror(errno));
            return;
        }

There are archs with sizeof(int) == 4 but a 64-bit address space. So, I
wonder whether the manpage for sysctl() isn't very precise or what's the
safe method for using sysctl with HW_USERMEM and HW_PHYSMEM.

        cachebufs = (usermem / MAXMEMPART) / (nblksread * dev_bsize);   

As usermem is an int cachebufs might have a negative value, now.

     } else {        /* User specified */
        cachebufs = cachesize;
    }

    if(cachebufs) { /* Don't allocate if zero --> no caching */
        if (cachebufs > MAXCACHEBUFS)
            cachebufs = MAXCACHEBUFS;

cachebufs might still have a negative value.

        sharedSize = sizeof(struct cheader) +
            sizeof(struct cdesc) * cachebufs +
            nblksread * cachebufs * dev_bsize;
#ifdef STATS
        fprintf(stderr, "Using %d buffers (%d bytes)\n", cachebufs,
            sharedSize);
#endif

size_t is not an int on every platform, so you must not use the printf
sequence %d with a size_t variable without casting it to int.

>How-To-Repeat:

>Fix:

Use more reasonable types like unsigned long long or at least uint64_t.
>Release-Note:
>Audit-Trail:

From: Manuel Bouyer <bouyer@antioche.eu.org>
To: Christian Biere <christianbiere@gmx.de>
Cc: gnats-bugs@gnats.netbsd.org
Subject: Re: bin/19852: Potential problem with dump and large amount of memory
Date: Wed, 15 Jan 2003 21:27:32 +0100

 On Tue, Jan 14, 2003 at 11:54:29PM +0100, Christian Biere wrote:
 > /usr/src/sbin/dump/rcache.c:
 > 
 > static int cachebufs;
 > 
 > [...]
 > 
 > void
 > initcache(int cachesize, int readblksize)
 > {
 >     size_t len;
 >     size_t  sharedSize;
 > 
 >     nblksread = (readblksize + ufsib->ufs_bsize - 1) / ufsib->ufs_bsize;
 >     if(cachesize == -1) {   /* Compute from memory available */
 >         int usermem;
 >         int mib[2] = { CTL_HW, HW_USERMEM };
 > 
 >         len = sizeof(usermem);
 >         if (sysctl(mib, 2, &usermem, &len, NULL, 0) < 0) {
 >             msg("sysctl(hw.usermem) failed: %s\n", strerror(errno));
 >             return;
 >         }
 > 
 > There are archs with sizeof(int) == 4 but a 64-bit address space. So, I
 > wonder whether the manpage for sysctl() isn't very precise or what's the
 > safe method for using sysctl with HW_USERMEM and HW_PHYSMEM.

 Well, HW_USERMEM is really returning an int.
 It looks like we need to redefine these sysctls.

 -- 
 Manuel Bouyer <bouyer@antioche.eu.org>
      NetBSD: 23 ans d'experience feront toujours la difference
 --
From: Christian Biere <christianbiere@gmx.de>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: bin/19852: Potential problem with dump and large amount of memory
Date: Sat, 10 Sep 2005 20:21:12 +0200

 Manuel Bouyer wrote:
 > On Tue, Jan 14, 2003 at 11:54:29PM +0100, Christian Biere wrote:
 > > /usr/src/sbin/dump/rcache.c:
 > > 
 > > static int cachebufs;

 Note that "cachebufs" is still of type "int" in the current code.

 > > void
 > > initcache(int cachesize, int readblksize)
 > > {
 > >     size_t len;
 > >     size_t  sharedSize;
 > > 
 > >     nblksread = (readblksize + ufsib->ufs_bsize - 1) / ufsib->ufs_bsize;
 > >     if(cachesize == -1) {   /* Compute from memory available */
 > >         int usermem;
 > >         int mib[2] = { CTL_HW, HW_USERMEM };

 They type of "usermem" is not "uint64_t" and "HW_USERMEM" was replaced
 with "HW_USERMEM64". I'm not 100% certain but to me it looks like
 "cachebufs" could *still* be negative on a system with a sufficiently
 high amount of RAM i.e., several Gigabyte which is certainly not unlikely
 on 64-bit server machines. Even if that's unlikely now, it's a time bomb.

 > Well, HW_USERMEM is really returning an int.
 > It looks like we need to redefine these sysctls.

 HW_USERMEM64 was introduced but this fix is most-likely incomplete
 as explained above.

 -- 
 Christian
 --
Module Name:    src
Committed By:   spz
Date:           Wed Jan 27 12:20:25 UTC 2010

Modified Files:
        src/sbin/dump: rcache.c

Log Message:
range-check what we assign to int cachebufs from calculations with
uint64_t usermem. This only becomes relevant if you have several TB of RAM.
Promoting cachebufs to uint64_t is not necessary as it gets limited to
(currently) 512 anyway.

fixes the last issue of PR: 19852


To generate a diff of this commit:
cvs rdiff -u -r1.22 -r1.23 src/sbin/dump/rcache.c


State-Changed-From-To: open->closed
State-Changed-By: spz@NetBSD.org
State-Changed-When: Wed, 27 Jan 2010 13:46:43 +0000
State-Changed-Why:
quite forward looking problems resulting in value range overrun of cachebufs
have been taken care of (usermem had been propagated to uint64_t and the
corresponding sysctl been using HW_USERMEM64 ages ago, which were more
immediate problems).


>Unformatted:

Home	PR Database Search