From ripley at stats.ox.ac.uk  Fri Jan 25 17:08:26 2008
From: ripley at stats.ox.ac.uk (Prof Brian Ripley)
Date: Fri, 25 Jan 2008 16:08:26 +0000 (GMT)
Subject: [Rd] strsignif.c, util.c (PR#10635)
In-Reply-To: <479A04B8.7010205@stats.uwo.ca>
References: <20080125132515.AF172283462C@mail.pubhealth.ku.dk>
	<479A04B8.7010205@stats.uwo.ca>
Message-ID: <Pine.LNX.4.64.0801251605130.17408@auk.stats>

On Fri, 25 Jan 2008, Duncan Murdoch wrote:

> On 1/25/2008 8:25 AM, ripley at stats.ox.ac.uk wrote:
>> On Fri, 25 Jan 2008, A.R.Runnalls at kent.ac.uk wrote:
>> 
>>> In R 2.6.1, a couple of places (discovered using valgrind) where the
>>> requested size of string buffers fails to account correctly for the
>>> trailing null byte:
>>> 
>>> 1. In src/appl/strsignif.c, 'f0' and 'form' at l. 108-9 each need at
>>> least 1 extra byte.
>>> 
>>> 2. In src/main/util.c, 'out' at l. 1081 needs at least one extra byte.
>>> 
>>> (Remember that the return value of strlen does not include the null byte.)
>> 
>> But it is subtler than that.  R_alloc contains the statement
>>
>>  	s = allocVector(RAWSXP, size + 1);
>> 
>> and so does over-allocate by at least one (there is a rounding up to a 
>> multiple of 8).  This is a historical anomaly (it used to allocate a 
>> CHARSXP that allowed for the null byte), but one which trying to eliminate 
>> caused too many crashes in package code.
>> 
>> I'd like to see the empirical evidence you have, as I have been unable to 
>> trigger an overrun here.
>
> That is not documented in Writing R Extensions or R Internals, so I think a 
> change is needed, either to the docs or the calls.  I've already changed 
> these calls.
>
> I'd rather keep the docs as they are, because they give a sensible definition 
> to the function.  If the implementation protects against sloppy usage that's 
> okay, but I don't think we should take advantage of it, in case some future 
> maintainer notices the inconsistency and removes it.

I'm not disagreeing, just saying the analysis appears to me to be 
incorrect, and so there may be a different bug than the one reported.

As I said, I have tried removing it and found ca 10 other places in R 
(which I fixed: acopy_string is recent) and ca 20 packages that relied on 
it.

-- 
Brian D. Ripley,                  ripley at stats.ox.ac.uk
Professor of Applied Statistics,  http://www.stats.ox.ac.uk/~ripley/
University of Oxford,             Tel:  +44 1865 272861 (self)
1 South Parks Road,                     +44 1865 272866 (PA)
Oxford OX1 3TG, UK                Fax:  +44 1865 272595



[Rd] strsignif.c, util.c (PR#10635)
Prof Brian Ripley