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

Fixed various type punning warnings by using memcpy() #172

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fixed various type punning warnings by using memcpy() #172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
46 changes: 35 additions & 11 deletions nifti2/nifti_tool.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ static int g_debug = 1;
#include <limits.h>
#include <string.h>
#include <stdint.h>
#include <assert.h> // TEMP
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this //TEP be removed before committing?


#include "nifti2_io.h"
#include "nifti_tool.h"
Expand Down Expand Up @@ -3828,7 +3829,6 @@ int modify_all_fields( void * basep, nt_opts * opts, field_s * fields, int flen)
*----------------------------------------------------------------------*/
int modify_field(void * basep, field_s * field, const char * data)
{
float fval;
const char * posn = data;
int val, max, fc, nchars;

Expand All @@ -3854,7 +3854,7 @@ int modify_field(void * basep, field_s * field, const char * data)

case DT_INT8:
{
max = 127;
max = INT8_MAX;
for( fc = 0; fc < field->len; fc++ )
{
if( sscanf(posn, " %d%n", &val, &nchars) != 1 )
Expand All @@ -3871,7 +3871,8 @@ int modify_field(void * basep, field_s * field, const char * data)
return 1;
}
/* otherwise, we're good */
(((char *)basep + field->offset))[fc] = (char)val;
char * offsetp = (char *)basep + field->offset;
offsetp[fc] = (char)val;
if( g_debug > 1 )
fprintf(stderr,"+d setting posn %d of '%s' to %d\n",
fc, field->name, val);
Expand All @@ -3882,7 +3883,7 @@ int modify_field(void * basep, field_s * field, const char * data)

case DT_INT16:
{
max = 32767;
max = INT16_MAX;
for( fc = 0; fc < field->len; fc++ )
{
if( sscanf(posn, " %d%n", &val, &nchars) != 1 )
Expand All @@ -3899,7 +3900,12 @@ int modify_field(void * basep, field_s * field, const char * data)
return 1;
}
/* otherwise, we're good */
((short *)((char *)basep + field->offset))[fc] = (short)val;
int16_t sval = (int16_t)val;
char * offsetp = (char *)basep + field->offset;
offsetp = &(offsetp[fc * sizeof(sval)]);
assert(offsetp == &(((short *)((char *)basep + field->offset))[fc])); // TEMP
assert(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this assert(0) be removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are the other assert()s going to stay? If so, that means some compiler warns about the original statements but not the asserts?
If the asserts will be kept, they should probably have casts to (char *)before the '&'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, going from a pointer to an array and back seems a little odd. What if 3900 were to read:
offsetp += fc*sizeof(sval); /* offset further by fc elements */

Or that could be merged into the previous line, though it is nice to spell out the pieces:

    /* from the struct base offset by the field location, access element #fc */
    char * offsetp = (char *)basep + field->offset + fc*sizeof(sval);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see my comments way above, from 2023-04-01, repeated here:

"Don't merge as-is, I added some asserts to verify the address calculation is correct. They are just there as a test and will be removed.

Alas, no test actually enters this code, as I confirmed with the assert(0)."

It would be good if we first added some tests that exercise this code. Then we can confirm the code change is correct by not hitting those asserts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, that was on me to do. Note that I was planning to add only a single test, not for every type. This probably won't happen today...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A single test would be great! Once you have a chance to do it, I'll run this code and check that those asserts don't fire, then clean up this PR for merging.

memcpy(offsetp, &sval, sizeof(sval));
if( g_debug > 1 )
fprintf(stderr,"+d setting posn %d of '%s' to %d\n",
fc, field->name, val);
Expand All @@ -3918,7 +3924,12 @@ int modify_field(void * basep, field_s * field, const char * data)
fc,field->len);
return 1;
}
((int *)((char *)basep + field->offset))[fc] = val;
int32_t ival = (int32_t)val;
char * offsetp = (char *)basep + field->offset;
offsetp = &(offsetp[fc * sizeof(ival)]);
assert(offsetp == &(((int *)((char *)basep + field->offset))[fc])); // TEMP
assert(0);
memcpy(offsetp, &ival, sizeof(ival));
if( g_debug > 1 )
fprintf(stderr,"+d setting posn %d of '%s' to %d\n",
fc, field->name, val);
Expand All @@ -3938,7 +3949,11 @@ int modify_field(void * basep, field_s * field, const char * data)
fc,field->len);
return 1;
}
((int64_t *)((char *)basep + field->offset))[fc] = v64;
char * offsetp = (char *)basep + field->offset;
offsetp = &(offsetp[fc * sizeof(v64)]);
assert(offsetp == &(((int64_t *)((char *)basep + field->offset))[fc])); // TEMP
assert(0);
memcpy(offsetp, &v64, sizeof(v64));
if( g_debug > 1 )
fprintf(stderr,"+d setting posn %d of '%s' to %" PRId64 "\n",
fc, field->name, v64);
Expand All @@ -3949,19 +3964,24 @@ int modify_field(void * basep, field_s * field, const char * data)

case DT_FLOAT32:
{
float f32;
for( fc = 0; fc < field->len; fc++ )
{
if( sscanf(posn, " %f%n", &fval, &nchars) != 1 )
if( sscanf(posn, " %f%n", &f32, &nchars) != 1 )
{
fprintf(stderr,"** found %d of %d modify values\n",
fc,field->len);
return 1;
}
/* otherwise, we're good */
((float *)((char *)basep + field->offset))[fc] = fval;
char * offsetp = (char *)basep + field->offset;
offsetp = &(offsetp[fc * sizeof(f32)]);
assert(offsetp == &(((float *)((char *)basep + field->offset))[fc])); // TEMP
assert(0);
memcpy(offsetp, &f32, sizeof(f32));
if( g_debug > 1 )
fprintf(stderr,"+d setting posn %d of '%s' to %f\n",
fc, field->name, fval);
fc, field->name, f32);
posn += nchars;
}
}
Expand All @@ -3979,7 +3999,11 @@ int modify_field(void * basep, field_s * field, const char * data)
return 1;
}
/* otherwise, we're good */
((double *)((char *)basep + field->offset))[fc] = f64;
char * offsetp = (char *)basep + field->offset;
offsetp = &(offsetp[fc * sizeof(f64)]);
assert(offsetp == &(((double *)((char *)basep + field->offset))[fc])); // TEMP
assert(0);
memcpy(offsetp, &f64, sizeof(f64));
if( g_debug > 1 )
fprintf(stderr,"+d setting posn %d of '%s' to %f\n",
fc, field->name, f64);
Expand Down
34 changes: 25 additions & 9 deletions niftilib/nifti1_tool.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ static int g_debug = 1;
#include <limits.h>
#include <string.h>
#include <stdint.h>
#include <assert.h> // TEMP

#include "nifti1_io.h"
#include "nifti1_tool.h"
Expand Down Expand Up @@ -2893,7 +2894,6 @@ int modify_all_fields( void * basep, nt_opts * opts, field_s * fields, int flen)
*----------------------------------------------------------------------*/
int modify_field(void * basep, field_s * field, const char * data)
{
float fval;
const char * posn = data;
int val, max, fc, nchars;

Expand All @@ -2919,7 +2919,7 @@ int modify_field(void * basep, field_s * field, const char * data)

case DT_INT8:
{
max = 127;
max = INT8_MAX;
for( fc = 0; fc < field->len; fc++ )
{
if( sscanf(posn, " %d%n", &val, &nchars) != 1 )
Expand All @@ -2936,7 +2936,8 @@ int modify_field(void * basep, field_s * field, const char * data)
return 1;
}
/* otherwise, we're good */
(((char *)basep + field->offset))[fc] = (char)val;
char * offsetp = (char *)basep + field->offset;
offsetp[fc] = (char)val;
if( g_debug > 1 )
fprintf(stderr,"+d setting posn %d of '%s' to %d\n",
fc, field->name, val);
Expand All @@ -2947,7 +2948,7 @@ int modify_field(void * basep, field_s * field, const char * data)

case DT_INT16:
{
max = 32767;
max = INT16_MAX;
for( fc = 0; fc < field->len; fc++ )
{
if( sscanf(posn, " %d%n", &val, &nchars) != 1 )
Expand All @@ -2964,7 +2965,12 @@ int modify_field(void * basep, field_s * field, const char * data)
return 1;
}
/* otherwise, we're good */
((short *)((char *)basep + field->offset))[fc] = (short)val;
int16_t sval = (int16_t)val;
char * offsetp = (char *)basep + field->offset;
offsetp = &(offsetp[fc * sizeof(sval)]);
assert(offsetp == &(((short *)((char *)basep + field->offset))[fc])); // TEMP
assert(0);
memcpy(offsetp, &sval, sizeof(sval));
if( g_debug > 1 )
fprintf(stderr,"+d setting posn %d of '%s' to %d\n",
fc, field->name, val);
Expand All @@ -2983,7 +2989,12 @@ int modify_field(void * basep, field_s * field, const char * data)
fc,field->len);
return 1;
}
((int *)((char *)basep + field->offset))[fc] = val;
int32_t ival = (int32_t)val;
char * offsetp = (char *)basep + field->offset;
offsetp = &(offsetp[fc * sizeof(ival)]);
assert(offsetp == &(((int *)((char *)basep + field->offset))[fc])); // TEMP
assert(0);
memcpy(offsetp, &ival, sizeof(ival));
if( g_debug > 1 )
fprintf(stderr,"+d setting posn %d of '%s' to %d\n",
fc, field->name, val);
Expand All @@ -2994,19 +3005,24 @@ int modify_field(void * basep, field_s * field, const char * data)

case DT_FLOAT32:
{
float f32;
for( fc = 0; fc < field->len; fc++ )
{
if( sscanf(posn, " %f%n", &fval, &nchars) != 1 )
if( sscanf(posn, " %f%n", &f32, &nchars) != 1 )
{
fprintf(stderr,"** found %d of %d modify values\n",
fc,field->len);
return 1;
}
/* otherwise, we're good */
((float *)((char *)basep + field->offset))[fc] = fval;
char * offsetp = (char *)basep + field->offset;
offsetp = &(offsetp[fc * sizeof(f32)]);
assert(offsetp == &(((float *)((char *)basep + field->offset))[fc])); // TEMP
assert(0);
memcpy(offsetp, &f32, sizeof(f32));
if( g_debug > 1 )
fprintf(stderr,"+d setting posn %d of '%s' to %f\n",
fc, field->name, fval);
fc, field->name, f32);
posn += nchars;
}
}
Expand Down