Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Changes from divest 1.1 #876

Merged
merged 3 commits into from
Oct 21, 2024
Merged

Changes from divest 1.1 #876

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b5faec4
Include tinydir.h earlier; avoid R-unfriendly calls
jonclayden Oct 21, 2024
6a3a4b4
Remove R special case now that stack usage is less
jonclayden Oct 21, 2024
cf9f981
Small fixes: out-of-bounds or NULL accesses, memory leaks, uninitiali…
jonclayden Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 12 additions & 46 deletions console/nii_dicom.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3963,7 +3963,7 @@ void clear_volume(struct TVolumeDiffusion *ptvd) {
ptvd->_dtiV[0] = -1;
for (int i = 1; i < 4; ++i)
ptvd->_dtiV[i] = 2;
for (int i = 1; i < 6; ++i)
for (int i = 0; i < 6; ++i)
ptvd->_symBMatrix[i] = NAN;
// numDti = 0;
} // clear_volume()
Expand Down Expand Up @@ -4211,31 +4211,6 @@ int fcmp(const void *a, const void *b) {
return 0;
}

#ifdef USING_R

// True iff dcm1 sorts *before* dcm2
bool compareTDCMdim(const TDCMdim &dcm1, const TDCMdim &dcm2) {
for (int i = MAX_NUMBER_OF_DIMENSIONS - 1; i >= 0; i--) {
if (dcm1.dimIdx[i] < dcm2.dimIdx[i])
return true;
else if (dcm1.dimIdx[i] > dcm2.dimIdx[i])
return false;
}
return false;
} // compareTDCMdim()

bool compareTDCMdimRev(const TDCMdim &dcm1, const TDCMdim &dcm2) {
for (int i = 0; i < MAX_NUMBER_OF_DIMENSIONS; i++) {
if (dcm1.dimIdx[i] < dcm2.dimIdx[i])
return true;
else if (dcm1.dimIdx[i] > dcm2.dimIdx[i])
return false;
}
return false;
} // compareTDCMdimRev()

#else

int compareTDCMdim(void const *item1, void const *item2) {
struct TDCMdim const *dcm1 = (const struct TDCMdim *)item1;
struct TDCMdim const *dcm2 = (const struct TDCMdim *)item2;
Expand All @@ -4261,8 +4236,6 @@ int compareTDCMdimRev(void const *item1, void const *item2) {
return 0;
} // compareTDCMdimRev()

#endif // USING_R

struct TDICOMdata readDICOMx(char *fname, struct TDCMprefs *prefs, struct TDTI4D *dti4D) {
// struct TDICOMdata readDICOMv(char * fname, int isVerbose, int compressFlag, struct TDTI4D *dti4D) {
int isVerbose = prefs->isVerbose;
Expand All @@ -4281,6 +4254,13 @@ struct TDICOMdata readDICOMx(char *fname, struct TDCMprefs *prefs, struct TDTI4D
dti4D->frameReferenceTime[0] = -1;
// dti4D->fragmentOffset[0] = -1;
dti4D->intenScale[0] = 0.0;
#ifdef USING_R
// Ensure dti4D fields are initialised, as in nii_readParRec()
for (int i = 0; i < kMaxDTI4D; i++) {
dti4D->S[i].V[0] = -1.0;
dti4D->TE[i] = -1.0;
}
#endif
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be unfenced if it's correct. There is branching elsewhere that depends on an otherwise uninitialised dti4D->S[0].V[0], which valgrind flags up, and this seemed like a reasonable place and reasonable values for initialisation based on the comparable nii_readParRec().

d.deID_CS_n = 0;
struct TVolumeDiffusion volDiffusion = initTVolumeDiffusion(&d, dti4D);
struct stat s;
Expand Down Expand Up @@ -4845,13 +4825,8 @@ struct TDICOMdata readDICOMx(char *fname, struct TDCMprefs *prefs, struct TDTI4D
// philDTI[i].V[0] = -1;
// array for storing DimensionIndexValues
int numDimensionIndexValues = 0;
#ifdef USING_R
// Allocating a large array on the stack, as below, vexes valgrind and may cause overflow
std::vector<TDCMdim> dcmDim(kMaxSlice2D);
#else
//don't use stack! TDCMdim dcmDim[kMaxSlice2D];
TDCMdim *dcmDim = (TDCMdim *)malloc(kMaxSlice2D * sizeof(TDCMdim));
#endif
for (int i = 0; i < kMaxSlice2D; i++) {
dcmDim[i].diskPos = i;
for (int j = 0; j < MAX_NUMBER_OF_DIMENSIONS; j++)
Expand Down Expand Up @@ -7464,7 +7439,7 @@ struct TDICOMdata readDICOMx(char *fname, struct TDCMprefs *prefs, struct TDTI4D
// LO array of floats stored in LONG STRING!
// [960.5\400\17.9108\0\-9999\-9999]
// we want 3rd value, e.g. 17.9:
float v[5];
float v[6];
dcmMultiFloat(lLength, (char *)&buffer[lPos], 5, v);
Copy link
Collaborator Author

@jonclayden jonclayden Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dcmMultiFloat() modifies elements 1 to n of its argument, so the array needs to have length n+1.

d.CSA.tablePos[3] = v[3] - tableDeltaGE;
d.CSA.tablePos[0] = 1.0;
Expand Down Expand Up @@ -8008,18 +7983,18 @@ struct TDICOMdata readDICOMx(char *fname, struct TDCMprefs *prefs, struct TDTI4D
mxIdx = i;
}
if (isVerbose > 1)
printf("slice %d is %gmm from isocenter\n", i, dx);
printMessage("slice %d is %gmm from isocenter\n", i, dx);
}
if (isVerbose > 1)
printf("slice %d is furthest (%gmm) from isocenter\n", mxIdx, mxDx);
printMessage("slice %d is furthest (%gmm) from isocenter\n", mxIdx, mxDx);
// second pass: measure each slices scalar distance from furthest slice
// ensure their are no repeated slice positions
fidx *objects = (fidx *)malloc(sizeof(struct fidx) * numDimensionIndexValues);
bool isSamePosition = false;
for (int i = 0; i < numDimensionIndexValues; i++) {
float dx = sqrt(pow(patientPosition1[i] - patientPosition1[mxIdx], 2) + pow(patientPosition2[i] - patientPosition2[mxIdx], 2) + pow(patientPosition3[i] - patientPosition3[mxIdx], 2));
if (isVerbose > 1)
printf("slice %d is %gmm from furthest slice\n", i, dx);
printMessage("slice %d is %gmm from furthest slice\n", i, dx);
objects[i].value = dx;
// sliceMM[i];
objects[i].index = i;
Expand Down Expand Up @@ -8104,17 +8079,10 @@ struct TDICOMdata readDICOMx(char *fname, struct TDCMprefs *prefs, struct TDTI4D
if ((isKludgeIssue533) && (numDimensionIndexValues > 1))
printWarning("Guessing temporal order for Philips enhanced DICOM ASL (issue 532).\n");
// sort dimensions
#ifdef USING_R
if (stackPositionItem < maxVariableItem)
std::sort(dcmDim.begin(), dcmDim.begin() + numberOfFrames, compareTDCMdim);
else
std::sort(dcmDim.begin(), dcmDim.begin() + numberOfFrames, compareTDCMdimRev);
#else
if (stackPositionItem < maxVariableItem)
qsort(dcmDim, numberOfFrames, sizeof(struct TDCMdim), compareTDCMdim);
else
qsort(dcmDim, numberOfFrames, sizeof(struct TDCMdim), compareTDCMdimRev);
#endif
// for (int i = 0; i < numberOfFrames; i++)
// printf("i %d diskPos= %d dimIdx= %d %d %d %d TE= %g\n", i, dcmDim[i].diskPos, dcmDim[i].dimIdx[0], dcmDim[i].dimIdx[1], dcmDim[i].dimIdx[2], dcmDim[i].dimIdx[3], dti4D->TE[i]);
for (int i = 0; i < numberOfFrames; i++) {
Expand Down Expand Up @@ -8470,9 +8438,7 @@ struct TDICOMdata readDICOMx(char *fname, struct TDCMprefs *prefs, struct TDTI4D
// printf("%g\t%g\t%s\n", d.intenIntercept, d.intenScale, fname);
if ((d.isLocalizer) && (strstr(d.seriesDescription, "b1map"))) // issue751 b1map uses same base as scout
d.isLocalizer = false;
#ifndef USING_R
free(dcmDim);
#endif
return d;
} // readDICOMx()

Expand Down
36 changes: 29 additions & 7 deletions console/nii_dicom_batch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@
#else
#undef MiniZ
#endif
#include "tinydir.h"
#include "nifti1_io_core.h"
#include "print.h"
#include "tinydir.h"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reordering avoids the DT_UNKNOWN symbol clash.

#ifndef USING_R
#include "nifti1.h"
#endif
Expand Down Expand Up @@ -1055,13 +1055,17 @@ int geProtocolBlock(const char *filename, int geOffset, int geLength, int isVerb
#ifdef myDisableMiniZ
ret = inflate(&s, Z_SYNC_FLUSH);
if (ret != Z_STREAM_END) {
free(pCmp);
free(pUnCmp);
inflateEnd(&s);
return EXIT_FAILURE;
}
#else
ret = mz_inflate(&s, MZ_SYNC_FLUSH);
if (ret != MZ_STREAM_END) {
free(pCmp);
free(pUnCmp);
inflateEnd(&s);
return EXIT_FAILURE;
}
#endif
Expand Down Expand Up @@ -1108,7 +1112,9 @@ int geProtocolBlock(const char *filename, int geOffset, int geLength, int isVerb
printMessage(" ViewOrder %d SliceOrder %d\n", *viewOrder, *sliceOrder);
printMessage("%s\n", pUnCmp);
}
free(pCmp);
free(pUnCmp);
inflateEnd(&s);
return EXIT_SUCCESS;
}
#endif // myReadGeProtocolBlock()
Expand Down Expand Up @@ -1629,7 +1635,7 @@ tse3d: T2*/
int iterations = 0;
// note, some vendors write 'OSEM3D-OP-PSFi10s16' others 'OP-OSEM4i21s'
// order matters `OP-OSEM4i21s` should have i=4 NOT i=21
bool sEnd = d.reconstructionMethod[strlen(d.reconstructionMethod) - 1] == 's';
bool sEnd = strlen(d.reconstructionMethod) == 0 ? false : (d.reconstructionMethod[strlen(d.reconstructionMethod) - 1] == 's');
for (int i = 1; i < 33; i++) {
char stri[12];
if (sEnd)
Expand Down Expand Up @@ -3678,7 +3684,9 @@ int nii_createFilename(struct TDICOMdata dcm, char *niiFilename, struct TDCMopts
strcat(bidsSubject, "1");
else
strcat(bidsSubject, opts.bidsSubject);
#ifndef USING_R
printf("%s<<<:::\n", bidsSubject);
#endif
char bidsSession[kOptsStr] = "ses-";
if (strlen(opts.bidsSession) <= 0)
strcat(bidsSession, "1");
Expand Down Expand Up @@ -5556,6 +5564,10 @@ int nii_saveNII(char *niiFilename, struct nifti_1_header hdr, unsigned char *im,
return EXIT_FAILURE;
image->data = (void *)im;
images->append(image, name);
// A copy of "image" has been made, so free all aspects of it except the data (which is owned by the caller)
free(image->fname);
free(image->iname);
nifti_free_extensions(image);
free(image);
return EXIT_SUCCESS;
}
Expand Down Expand Up @@ -6428,6 +6440,7 @@ int nii_saveCrop(char *niiFilename, struct nifti_1_header hdr, unsigned char *im
strcat(niiFilenameCrop, "_Crop");
const int returnCode = nii_saveNII3D(niiFilenameCrop, hdrX, imX, opts, d);
free(imX);
free(sliceSums);
return returnCode;
} // nii_saveCrop()

Expand Down Expand Up @@ -6767,16 +6780,20 @@ void readSoftwareVersionsGE(char softwareVersionsGE[], int verbose, char geVersi
sepStart += 1;
}
}
len = 11; // RX27.0_R02_
// Default: start from the beginning
if (ismatched == false) {
sepStart = softwareVersionsGE;
}
len = 12; // RX27.0_R02_, plus nul terminator
char *versionString = (char *)malloc(sizeof(char) * len);
versionString[len - 1] = 0;
memcpy(versionString, sepStart, len);
memcpy(versionString, sepStart, len-1);
char c1, c2, c3, c4;
// RX27.0_R02_ or MR29.1_EA_2
sscanf(versionString, "%c%c%d.%d_%c%c%d", &c1, &c2, geMajorVersionInt, geMinorVersionInt, &c3, &c4, geReleaseVersionInt);
int fields = sscanf(versionString, "%c%c%d.%d_%c%c%d", &c1, &c2, geMajorVersionInt, geMinorVersionInt, &c3, &c4, geReleaseVersionInt);
memcpy(geVersionPrefix, &c1, 1);
memcpy(geVersionPrefix + 1, &c2, 1);
if ((c3 == 'E') && (c4 == 'A')) {
if ((fields >= 6) && (c3 == 'E') && (c4 == 'A')) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version string 14.0_M4_062 appears in the QA battery, and doesn't match the sscanf() call, so c3 and c4 end up uninitialised. This is defensive programming to check that enough fields were read.

*geReleaseVersionInt = 0;
}
free(versionString);
Expand Down Expand Up @@ -7098,7 +7115,7 @@ void setBidsSiemens(struct TDICOMdata *d, int nConvert, int isVerbose, const cha
strcat(suffixBIDS, modalityBIDS);
}
if ((isVerbose > 0) || (strlen(dataTypeBIDS) < 1))
printf("::autoBids:Siemens CSAseqFname:'%s' pulseSeq:'%s' seqName:'%s'\n",
printMessage("::autoBids:Siemens CSAseqFname:'%s' pulseSeq:'%s' seqName:'%s'\n",
seqDetails, d->pulseSequenceName, d->sequenceName);
if (isDerived)
strcpy(dataTypeBIDS, "derived");
Expand Down Expand Up @@ -8845,8 +8862,12 @@ int saveDcm2Nii(int nConvert, struct TDCMsort dcmSort[], struct TDICOMdata dcmLi
// issue867 TODO sizeof(TDTI4D) = 10mb, this could be reduced if elements reduced from kMaxDTI4D
TDTI4D *dti4Ds = (TDTI4D *)malloc(sizeof(TDTI4D));
if (dti4Ds == NULL) {
#ifdef USING_R
Rf_error("Failed to allocate memory for dti4Ds");
#else
perror("Failed to allocate memory for dti4Ds");
exit(EXIT_FAILURE);
#endif
}
// Copy the content from the original dti4D into dti4Ds
*dti4Ds = *dti4D;
Expand Down Expand Up @@ -9503,6 +9524,7 @@ int searchDirRenameDICOM(char *path, int maxDepth, int depth, struct TDCMopts *o
}
}
}
tinydir_close(&dir);
return count;
}

Expand Down
Loading