Skip to content

Commit

Permalink
memcpy checks only where required, gcc5.3 UBSAN passes, #1348
Browse files Browse the repository at this point in the history
  • Loading branch information
@arunsrinivasan
arunsrinivasan committed Mar 10, 2016
1 parent 086a326 commit 6f62db6
Show file tree
Hide file tree
Showing 7 changed files with 22 additions and 26 deletions.
8 changes: 4 additions & 4 deletions src/assign.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -747,11 +747,11 @@ void memrecycle(SEXP target, SEXP where, int start, int len, SEXP source)
REAL(target)[start+r] = REAL(source)[r%slen];
} else {
for (r=r>0?1:0; r<(len/slen); r++) { // if the first slen were done in the switch above, convert r=slen to r=1
if (slen) memcpy((char *)DATAPTR(target) + (start+r*slen)*size,
memcpy((char *)DATAPTR(target) + (start+r*slen)*size,
(char *)DATAPTR(source),
slen * size);
}
if (len%slen) memcpy((char *)DATAPTR(target) + (start+r*slen)*size,
memcpy((char *)DATAPTR(target) + (start+r*slen)*size,
(char *)DATAPTR(source),
(len%slen) * size);
}
Expand Down Expand Up @@ -896,12 +896,12 @@ SEXP setcolorder(SEXP x, SEXP o)
SEXP *tmp = Calloc(LENGTH(x),SEXP);
for (int i=0; i<LENGTH(x); i++)
tmp[i] = VECTOR_ELT(x, INTEGER(o)[i]-1);
if (LENGTH(x)) memcpy((char *)DATAPTR(x),(char *)tmp,LENGTH(x)*sizeof(char *)); // sizeof is of type size_t (unsigned) - so no overflow here
memcpy((char *)DATAPTR(x),(char *)tmp,LENGTH(x)*sizeof(char *)); // sizeof is of type size_t (unsigned) - so no overflow here
SEXP names = getAttrib(x,R_NamesSymbol);
if (isNull(names)) error("dt passed to setcolorder has no names");
for (int i=0; i<LENGTH(x); i++)
tmp[i] = STRING_ELT(names, INTEGER(o)[i]-1);
if (LENGTH(x)) memcpy((char *)DATAPTR(names),(char *)tmp,LENGTH(x)*sizeof(char *));
memcpy((char *)DATAPTR(names),(char *)tmp,LENGTH(x)*sizeof(char *));
// No need to change key (if any); sorted attribute is column names not positions
Free(tmp);
return(R_NilValue);
Expand Down
4 changes: 2 additions & 2 deletions src/dogroups.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ SEXP dogroups(SEXP dt, SEXP dtcols, SEXP groups, SEXP grpcols, SEXP jiscols, SEX
rownum = INTEGER(starts)[i]-1;
for (j=0; j<length(SDall); j++) {
size = SIZEOF(VECTOR_ELT(SDall,j));
if (grpn) memcpy((char *)DATAPTR(VECTOR_ELT(SDall,j)), // direct memcpy best here, for usually large size groups. by= each row is slow and not recommended anyway, so we don't mind there's no switch here for grpn==1
memcpy((char *)DATAPTR(VECTOR_ELT(SDall,j)), // direct memcpy best here, for usually large size groups. by= each row is slow and not recommended anyway, so we don't mind there's no switch here for grpn==1
(char *)DATAPTR(VECTOR_ELT(dt,INTEGER(dtcols)[j]-1))+rownum*size,
grpn*size);
// SD is our own alloc'd memory, and the source (DT) is protected throughout, so no need for SET_* overhead
Expand Down Expand Up @@ -537,7 +537,7 @@ SEXP growVector(SEXP x, R_len_t newlen)
// TO DO: Again, is there bulk op to avoid this loop, which still respects older generations
break;
default :
if (len) memcpy((char *)DATAPTR(newx), (char *)DATAPTR(x), len*SIZEOF(x)); // SIZEOF() returns size_t (just as sizeof()) so * shouldn't overflow
memcpy((char *)DATAPTR(newx), (char *)DATAPTR(x), len*SIZEOF(x)); // SIZEOF() returns size_t (just as sizeof()) so * shouldn't overflow
}
// if (verbose) Rprintf("Growing vector from %d to %d items of type '%s'\n", len, newlen, type2char(TYPEOF(x)));
// Would print for every column if here. Now just up in dogroups (one msg for each table grow).
Expand Down
14 changes: 7 additions & 7 deletions src/fmelt.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -438,23 +438,23 @@ SEXP getvaluecols(SEXP DT, SEXP dtnames, Rboolean valfactor, Rboolean verbose, s
for (k=0; k<thislen; k++)
REAL(target)[counter + k] = REAL(thiscol)[INTEGER(thisidx)[k]-1];
} else {
if (data->nrow) memcpy((char *)DATAPTR(target)+j*data->nrow*size, (char *)DATAPTR(thiscol), data->nrow*size);
memcpy((char *)DATAPTR(target)+j*data->nrow*size, (char *)DATAPTR(thiscol), data->nrow*size);
}
break;
case INTSXP :
if (data->narm) {
for (k=0; k<thislen; k++)
INTEGER(target)[counter + k] = INTEGER(thiscol)[INTEGER(thisidx)[k]-1];
} else {
if (data->nrow) memcpy((char *)DATAPTR(target)+j*data->nrow*size, (char *)DATAPTR(thiscol), data->nrow*size);
memcpy((char *)DATAPTR(target)+j*data->nrow*size, (char *)DATAPTR(thiscol), data->nrow*size);
}
break;
case LGLSXP :
if (data->narm) {
for (k=0; k<thislen; k++)
LOGICAL(target)[counter + k] = LOGICAL(thiscol)[INTEGER(thisidx)[k]-1];
} else {
if (data->nrow) memcpy((char *)DATAPTR(target)+j*data->nrow*size, (char *)DATAPTR(thiscol), data->nrow*size);
memcpy((char *)DATAPTR(target)+j*data->nrow*size, (char *)DATAPTR(thiscol), data->nrow*size);
}
break;
default : error("Unknown column type '%s' for column '%s'.", type2char(TYPEOF(thiscol)), CHAR(STRING_ELT(dtnames, INTEGER(thisvaluecols)[i]-1)));
Expand Down Expand Up @@ -575,7 +575,7 @@ SEXP getidcols(SEXP DT, SEXP dtnames, Rboolean verbose, struct processData *data
}
} else {
for (j=0; j<data->lmax; j++)
if (data->nrow) memcpy((char *)DATAPTR(target)+j*data->nrow*size, (char *)DATAPTR(thiscol), data->nrow*size);
memcpy((char *)DATAPTR(target)+j*data->nrow*size, (char *)DATAPTR(thiscol), data->nrow*size);
}
break;
case INTSXP :
Expand All @@ -589,7 +589,7 @@ SEXP getidcols(SEXP DT, SEXP dtnames, Rboolean verbose, struct processData *data
}
} else {
for (j=0; j<data->lmax; j++)
if (data->nrow) memcpy((char *)DATAPTR(target)+j*data->nrow*size, (char *)DATAPTR(thiscol), data->nrow*size);
memcpy((char *)DATAPTR(target)+j*data->nrow*size, (char *)DATAPTR(thiscol), data->nrow*size);
}
break;
case LGLSXP :
Expand All @@ -603,7 +603,7 @@ SEXP getidcols(SEXP DT, SEXP dtnames, Rboolean verbose, struct processData *data
}
} else {
for (j=0; j<data->lmax; j++)
if (data->nrow) memcpy((char *)DATAPTR(target)+j*data->nrow*size, (char *)DATAPTR(thiscol), data->nrow*size);
memcpy((char *)DATAPTR(target)+j*data->nrow*size, (char *)DATAPTR(thiscol), data->nrow*size);
}
break;
case STRSXP :
Expand All @@ -620,7 +620,7 @@ SEXP getidcols(SEXP DT, SEXP dtnames, Rboolean verbose, struct processData *data
// From assign.c's memcrecycle - only one SET_STRING_ELT per RHS item is needed to set generations (overhead)
for (k=0; k<data->nrow; k++) SET_STRING_ELT(target, k, STRING_ELT(thiscol, k));
for (j=1; j<data->lmax; j++)
if (data->nrow) memcpy((char *)DATAPTR(target)+j*data->nrow*size, (char *)DATAPTR(target), data->nrow*size);
memcpy((char *)DATAPTR(target)+j*data->nrow*size, (char *)DATAPTR(target), data->nrow*size);
}
break;
case VECSXP :
Expand Down
16 changes: 6 additions & 10 deletions src/forder.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -386,10 +386,8 @@ static void iradix_r(int *xsub, int *osub, int n, int radix)
otmp[j] = osub[i];
((int *)xtmp)[j] = xsub[i];
}
if (n) {
memcpy(osub, otmp, n*sizeof(int));
memcpy(xsub, xtmp, n*sizeof(int));
}
memcpy(osub, otmp, n*sizeof(int));
memcpy(xsub, xtmp, n*sizeof(int));

nextradix = radix-1;
while (nextradix>=0 && skip[nextradix]) nextradix--;
Expand Down Expand Up @@ -670,10 +668,8 @@ static void dradix_r(unsigned char *xsub, int *osub, int n, int radix)
p -= colSize;
}
}
if (n) {
memcpy(osub, otmp, n*sizeof(int));
memcpy(xsub, xtmp, n*colSize);
}
memcpy(osub, otmp, n*sizeof(int));
memcpy(xsub, xtmp, n*colSize);

nextradix = radix-1;
while (nextradix>=0 && skip[nextradix]) nextradix--;
Expand Down Expand Up @@ -770,7 +766,7 @@ static void cradix_r(SEXP *xsub, int n, int radix)
j = --thiscounts[thisx];
cradix_xtmp[j] = xsub[i];
}
if (n) memcpy(xsub, cradix_xtmp, n*sizeof(SEXP));
memcpy(xsub, cradix_xtmp, n*sizeof(SEXP));
if (radix == maxlen-1) {
memset(thiscounts, 0, 256*sizeof(int));
return;
Expand Down Expand Up @@ -1272,7 +1268,7 @@ SEXP forder(SEXP DT, SEXP by, SEXP retGrp, SEXP sortStrArg, SEXP orderArg, SEXP
if (newo[0] != -1) {
if (nalast != 0) for (j=0; j<thisgrpn; j++) ((int *)xsub)[j] = osub[ newo[j]-1 ]; // reuse xsub to reorder osub
else for (j=0; j<thisgrpn; j++) ((int *)xsub)[j] = (newo[j] == 0) ? 0 : osub[ newo[j]-1 ]; // final nalast case to handle!
if (thisgrpn) memcpy(osub, xsub, thisgrpn*sizeof(int));
memcpy(osub, xsub, thisgrpn*sizeof(int));
}
TEND(6)
}
Expand Down
2 changes: 1 addition & 1 deletion src/rbindlist.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -734,7 +734,7 @@ SEXP rbindlist(SEXP l, SEXP sexp_usenames, SEXP sexp_fill, SEXP idcol) {
case INTSXP:
case LGLSXP:
if (TYPEOF(thiscol) != TYPEOF(target)) error("Internal logical error in rbindlist.c, type of 'thiscol' should have already been coerced to 'target'. Please report to datatable-help.");
if (thislen) memcpy((char *)DATAPTR(target) + ansloc * SIZEOF(thiscol),
memcpy((char *)DATAPTR(target) + ansloc * SIZEOF(thiscol),
(char *)DATAPTR(thiscol),
thislen * SIZEOF(thiscol));
break;
Expand Down
2 changes: 1 addition & 1 deletion src/reorder.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ SEXP reorder(SEXP x, SEXP order)
tmpp += 8;
}
}
if (end-start+1) memcpy(vd + start*size, tmp, (end-start+1) * size);
memcpy(vd + start*size, tmp, (end-start+1) * size);
}
free(tmp);
return(R_NilValue);
Expand Down
2 changes: 1 addition & 1 deletion src/uniqlist.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ SEXP uniqlist(SEXP l, SEXP order)
}
}
PROTECT(ans = allocVector(INTSXP, len));
if (len) memcpy(INTEGER(ans), iidx, sizeof(int)*len); // sizeof is of type size_t - no integer overflow issues
memcpy(INTEGER(ans), iidx, sizeof(int)*len); // sizeof is of type size_t - no integer overflow issues
Free(iidx);
UNPROTECT(1);
return(ans);
Expand Down

0 comments on commit 6f62db6

Please sign in to comment.