search

MichaelChirico / r-bugs

A ⚠️read-only⚠️mirror of https://bugs.r-project.org/
20 stars 0 forks source link

[BUGZILLA #10635] strsignif.c, util.c #3264

Closed MichaelChirico closed 4 years ago

MichaelChirico commented 4 years ago

From: Andrew Runnalls <A.R.Runnalls@<::CENSORED -- SEE ORIGINAL ON BUGZILLA::>> 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.)

Andrew Runnalls

METADATA

MichaelChirico commented 4 years ago

From: Duncan Murdoch <murdoch@<::CENSORED -- SEE ORIGINAL ON BUGZILLA::>> A.R.Runnalls@<::CENSORED -- SEE ORIGINAL ON BUGZILLA::> 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.

At least this one is still there; I'll fix it (and take a look at the other, too).

Thanks!

Duncan Murdoch

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.)

Andrew Runnalls

______________________________________________
R-devel@<::CENSORED -- SEE ORIGINAL ON BUGZILLA::>-project.org mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel

METADATA

MichaelChirico commented 4 years ago

From: Prof Brian Ripley <ripley@<::CENSORED -- SEE ORIGINAL ON BUGZILLA::>> On Fri, 25 Jan 2008, A.R.Runnalls@<::CENSORED -- SEE ORIGINAL ON BUGZILLA::> 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.

-- Brian D. Ripley, ripley@<::CENSORED -- SEE ORIGINAL ON BUGZILLA::> Professor of Applied Statistics, http://www.stats.ox.ac.uk/~ripley/ <CENSORING FROM DETECTED PHONE NUMBER ONWARDS; SEE BUGZILLA>

METADATA

MichaelChirico commented 4 years ago

From: Duncan Murdoch <murdoch@<::CENSORED -- SEE ORIGINAL ON BUGZILLA::>> On 1/25/2008 8:25 AM, ripley@<::CENSORED -- SEE ORIGINAL ON BUGZILLA::> wrote:

On Fri, 25 Jan 2008, A.R.Runnalls@<::CENSORED -- SEE ORIGINAL ON BUGZILLA::> 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.

Duncan Murdoch

METADATA

MichaelChirico commented 4 years ago

From: Andrew Runnalls <A.R.Runnalls@<::CENSORED -- SEE ORIGINAL ON BUGZILLA::>> Prof Brian Ripley wrote:

On Fri, 25 Jan 2008, A.R.Runnalls@<::CENSORED -- SEE ORIGINAL ON BUGZILLA::> 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.

I should perhaps have explained that I discovered these faults working on a modified code base, in which in particular the valgrind instrumentation will pick up an overrun of even a single byte over the requested block size. I have no reason to suppose that the faults will lead to a failure of R as currently engineered; indeed for the reasons you cite I guess they won't.

However, buffer overruns - when they do manifest themselves - can give rise to such baffling and hard-to-reproduce failures that they are like submerged rocks: well worth marking on the chart even if they're not close to current shipping lanes.

Andrew Runnalls

-- Dr Andrew Runnalls, Computing Laboratory, University of Kent, CANTERBURY CT2 7NF, UK

<CENSORING FROM DETECTED PHONE NUMBER ONWARDS; SEE BUGZILLA>

METADATA

MichaelChirico commented 4 years ago

Audit (from Jitterbug): Tue Jan 29 19:32:00 2008 ripley changed notes Tue Jan 29 18:32:00 2008 ripley moved from incoming to Misc-fixed

METADATA

MichaelChirico commented 4 years ago

NOTES: Not a bug in R, but in private build. Changes anyway for 2.7.0

METADATA