[Rcpp-devel] ColDatum constructors memory safety

[Dirk Eddelbuettel]

On 16 March 2010 at 16:59, Alistair Gee wrote:
| Here are patches to RcppFrame.h:

Sweet!  Thanks a lot!  

Now, just to top it off, could provide a unit test that would ideally trigger
the issue (or something related) pre-fix but not post-fix?

Dirk
 
| --- RcppFrame.h~        2010-02-16 10:54:01.000000000 -0500
| +++ RcppFrame.h 2010-03-16 19:48:29.847994574 -0400
| @@ -30,7 +30,8 @@
|  enum ColType {                 // Supported data frame column types.
|      COLTYPE_DOUBLE, COLTYPE_INT, COLTYPE_STRING,
|      COLTYPE_FACTOR, COLTYPE_LOGICAL,
| -    COLTYPE_DATE, COLTYPE_DATETIME
| +    COLTYPE_DATE, COLTYPE_DATETIME,
| +    COLTYPE_UNKNOWN = -1
|  };
|  class ColDatum;                        // forward declaration, see below
| 
| 
| and RcppFrame.cpp:
| 
| 
| --- RcppFrame.cpp~      2010-03-12 15:11:45.000000000 -0500
| +++ RcppFrame.cpp       2010-03-16 19:48:30.517076368 -0400
| @@ -22,7 +22,7 @@
| 
|  #include <RcppFrame.h>
| 
| -ColDatum::ColDatum() : level(0) { }
| +ColDatum::ColDatum() : type(COLTYPE_UNKNOWN), level(0) { }
| 
|  ColDatum::ColDatum(const ColDatum& datum) {
|      // Need deep copy so contruction/destruction is synchronized.
| 
| 
| 
| 
| On Sat, Mar 13, 2010 at 12:03 PM, Dirk Eddelbuettel <edd at debian.org> wrote:
| >
| > Hi Alistair,
| >
| > Thanks for sharing the analysis.
| >
| > On 12 March 2010 at 15:15, Alistair Gee wrote:
| > | The ColDatum default constructor does not initialize any fields except
| > | for "level". But the ColDatum *copy* constructor expects the "type"
| > | field to be properly initialized, b/c if type is COLTYPE_FACTOR, the
| > | copy constructor proceeds to allocate memory using the numLevels field
| > | (which is unreliable b/c numLevels was never initialized). See
| > | comments in source below:
| > |
| > | ColDatum::ColDatum() : level(0) { };
| > |
| > | ColDatum::ColDatum(const ColDatum& datum) {
| > |   // Need deep copy so contruction/destruction is synchronized.
| > |   s = datum.s;
| > |   x = datum.x;
| > |   i = datum.i;
| > |   type = datum.type;                // XXX datum.type was never initialized.
| > |   level = datum.level;
| > |   numLevels = datum.numLevels; // XXX datum.numLevels was never initialzed.
| > |   d = datum.d;
| > |   if (type == COLTYPE_FACTOR) {
| > |       levelNames = new std::string[numLevels];   // XXX numLevels was copied
| > |
| > | // from datum.numLevels which
| > |
| > | // was never initialized
| > |       for (int j = 0; j < numLevels; j++)
| > |           levelNames[j] = datum.levelNames[j];     // XXX datum.levelNames is
| > |
| > | // an uninitialized pointer.
| > |   }
| > | }
| > |
| > | This is a problem if one tries to create a vector of ColDatum, e.g.
| > |
| > | vector<ColDatum> colDatumVector(10);
| > |
| > | Here, the default constructor is used to first construct a ColDatum
| > | object (which will not have "type" and "levelNames" properly
| > | initialized). Then the copy constructor called by the vector
| > | constructor to create 10 copies.
| >
| > ColDatum is part of what we now call the 'classic' API. It is not getting
| > much attention or love as we focus on the 'new' API. That said, the new API
| > still lacks coverage for data.frame (and Date or Datetime classes).
| >
| > If you can think of a fix, we'd give it a good luck.
| >
| > Else, if you had a patch to clarify the header or code file and maybe just
| > warn of the issue, that would help too.
| >
| > Cheers, Dirk
| >
| > --
| >  Registration is open for the 2nd International conference R / Finance 2010
| >  See http://www.RinFinance.com for details, and see you in Chicago in April!
| >

-- 
  Registration is open for the 2nd International conference R / Finance 2010
  See http://www.RinFinance.com for details, and see you in Chicago in April!