2015.01.06 04:17 "Re: [Tiff] [PATCH] tif_luv, tif_pixarlog, ppm2tiff: get rid of duplicates of TIFFSafeMultiply", by Yuriy M. Kaminskiy

2014-12-31 11:04 GMT-05:00 Bob Friesenhahn <bfriesen@simple.dallas.tx.us>:

Whatever is done, it would be good if the approach is normalized rather than creating many one-of solutions in the code.

I agree, as soon as we can figure out what to normalize it too :-)

For now, I have fixed checkAdd64 since it was plain nonsense. I have left the _ms functions alone since, as Yuriy pointed out, the multiply is also unreliable because of undefined behavior (of signed int overflow). This means TIFFSafeMultiply with tmsize_t is not a proper solution either as currently written.

I think the easist fix for those would be to do the math with unsigned integers after validation that the input is nonnegative. Full check of

If input is non-negative, IMO, better check for multiplication overflow is

  (a) > type##_MAX/((b) | ((b) == 0))? 0: (a)*(b)

Same with addition overflow (again, assuming non-negative a and b):

  (a) > type##_MAX - (b)? 0: (a)+(b)

(assuming tmsize_t_MAX, uint32_MAX, etc was added to tiffconf.h.in)

BTW, while checking for other "duplicates of TIFFSafeMultiply" I also noticed *a lot* of places with code like:

  uint64 foo;
  tmsize_t bar;
  ...
  bar = (tmsize_t)foo;
  if ((uint64)bar != foo) {
     /* throw integer overflow */
  }

Obviously, this check will fail to catch "foo > INT64_MAX" on 64-bit platform; and I'm not totally sure about overly "smart" compilers on 32-bit platforms either. IMO, this should be replaced with

   if (foo > tmsize_t_MAX) {
     /* throw integer overflow */
   }
   bar = foo;

signed integer multiply is also possible but it is more code than I'm comfortable putting in a macro.

There is value gained from using pre-processor macros in that they do not influence the values type. Hard-coded functions may produce an unexpected result if the function prototype converts the input value to a different type. For example, if size_t is assumed, then there could be a problem if the computation is intentionally using types larger than size_t, or a signed type.

Macros can be misused in many ways as well. As pointed out above, you don't know the data type in the macro and the code needed to detect overflow should be different for signed/unsigned data. It gets even worse if you accidentally mix signed and unsigned. I'm not sure C offers any good solution to prevent programmer error here. It all comes down to using the macro/function properly.

With that said, as libtiff has fairly few use cases, I think we could write different macros to handle them (ie. tmsize_t multiply, uint32 multiply, uint64 multiply, ...). Hopefully someone will have a better idea.