On 18 March 2010 at 21:04, Alistair Gee wrote:
| I'm not sure that you can verify the fix with an empirical test, short
| of using something like valgrind b/c the bug would occur only due to
| chance:
|
| Here is a simpler scenario:
|
| ColDatum c1; // via the default constructor
| ColDatum c2(c1); // via the copy constructor
|
| When c1 is constructed via the default constructor (and without my
| patch), only the level field is initialized. The other fields, such as
| type, numLevels, and levelNames are not initialized, so they can be
| garbage values. So, if by chance, c1.type == COLTYPE_FACTOR, then when
| c2 is constructed by the copy constructor, the code to copy the
| levelNames is executed, which proceeds to copy the data
| c1.levelNames[0] .. c1.levelNames[c1.numLevels - 1]. But c1.levelNames
| wasn't initialized, so it could refer to random memory. Moreover,
| c1.numLevels wasn't initialized either, so it could be anything as
| well. Thus, the code could be accessing invalid memory.
|
| Here is the code to the copy constructor:
|
| 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;
| level = datum.level;
| numLevels = datum.numLevels;
| d = datum.d;
| if (type == COLTYPE_FACTOR) {
| levelNames = new std::string[numLevels];
| for (int j = 0; j < numLevels; j++)
| levelNames[j] = datum.levelNames[j];
| }
| }
|
| Note also the destructor. When c1's destructor is invoked, and c1.type
| == COLTYPE_FACTOR, the code will attempt to delete [] levelNames but
| that field was never initialized, so the code would attempt to
| incorrectly deallocate memory:
|
| ColDatum::~ColDatum() {
| if (type == COLTYPE_FACTOR) {
| // For this to work we need a deep copy when type == COLTYPE_FACTOR.
| // See the copy constructor below. It is wasteful to have
| // evey factor cell own a separate copy of levelNames, but we leave
| // the task of factoring it out (using reference counts) for
| // later.
| delete [] levelNames;
| }
| }
|
| The patch fixes both problems by initializing c1.type to
| COLTYPE_UNKNOWN (which I added to the enumeration).
All good points. I'll commit the patch, and expand the unit test a little
more to add a type or two more among the ones that ColDatum provides.
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!
_______________________________________________
Rcpp-devel mailing list
[email protected]
https://lists.r-forge.r-project.org/cgi-bin/mailman/listinfo/rcpp-devel
