[Insight-developers] snprintf portability (was: Fix possibility of buffer overflow in itkNumericSeriesF...)

[Bill Lorensen]

I think we have beaten this horse to death. The patch is greatly
improved from the first one and solves most problems. I vote that we
merge if one of of verifies.

Bill


On Wed, Jan 19, 2011 at 3:18 PM, Michael Stauffer <mstauff at verizon.net> wrote:
> I just submitted a small change to address one of Brad's comments.
>
> I'm fine with the patch as it is, warts and all. I could implement the formatting manually as suggested previously, or something like Norma's suggestion below. I don't feel I have the authority or ITK experience to make a call here - is there anyone who makes a final call in on-going situations like these?
>
> -Michael
>
>
> Jan 19, 2011 12:36:30 PM, bill.lorensen at gmail.com wrote:
>
> ===========================================
>
> I think the current gerrit patch will work fine on the compilers that
> ITK supports.
> http://review.source.kitware.com/#change,521
>
> Bill
>
> On Wed, Jan 19, 2011 at 11:54 AM, Williams, Norman K
>  wrote:
>> Given all the trouble around snprintf -- and I would NEVER have gone down
>> that road if I'd known what a mess it was going to make -- I wonder if we
>> should consider CHANGING THE FRICKING CLASS.
>>
>> It wasn't a great idea in the first place to use a printf format string as
>> an argument to an ITK class method.  C++ is theoretically supposed to be a
>> post-printf world*  It exposes the wrong level of abstraction. It's an
>> invitation to users to diddle the underlying implementation, not an
>> explicit contract of promised behavior.
>>
>> As a 'coffee' cup proposal, maybe something like this would be at a more
>> appropriate level of abstraction
>>
>> /** the part of the filenames preceding the file number */
>> void SetPrefix(const std::string &prefix);
>> /** the part of the filenams following the file number */
>> void SetSuffix(const std::string &suffix);
>> /** how many digits, minimum to represent the file number */
>> void SetIntegerWidth(int width);
>> /** whether to zero-pad the file number */
>> void ZeroPad(bool pad);
>>
>> *of course, people still use printf formatting in C++, and I don't even
>> know if standard C++ even provides all the formatting options available by
>> printf format strings. It's a significant failure of the C++ language &
>> library that the standard hasn't comprehensively made printf obsolete, in
>> a completely portable, well defined way.
>>
>> On 1/19/11 10:01 AM, "Brad King"  wrote:
>>
>>>On 01/19/2011 10:49 AM, Code Review wrote:
>>>> From Bill Lorensen :
>>>> I think a better job could be done for windows. Currently, the first
>>>>snprintf
>>>> will always return -1 for windows.  If char c[10000]; was used instead
>>>>of char c;
>>>> most of the time (except for huge filenames) snprintf will return a
>>>>valid value
>>>> on all platforms.
>>>> To view, visit http://review.source.kitware.com/521
>>>
>>>In that case we might as well just use that result directly and only
>>>fall back to dynamic allocation when the stack buffer isn't big enough.
>>>
>>>For reference, I just experimented with snprintf and _snprintf on a
>>>bunch of compilers:
>>>
>>>  - HP
>>>  - SunPro
>>>  - SGI MipsPro
>>>  - IBM XL
>>>  - GNU on Linux
>>>  - Intel on Linux
>>>  - Intel on Windows
>>>  - VS 6, 7, 7.1, 8, 9, 10
>>>  - Borland 5.5, 5.6, 5.8
>>>  - Watcom
>>>  - GNU on Windows (MinGW)
>>>  - GNU on Cygwin (default, -mwin32, -c99, -mwin32 -c99)
>>>
>>>The attached snprintf.c shows the results I encountered.  The most
>>>interesting was on SGI where -D_XOPEN_SOURCE=500 changes the result.
>>>Only cases in which the buffer is large enough are consistent everywhere.
>>>
>>>-Brad
>>
>> _______________________________________________
>> Powered by www.kitware.com
>>
>> Visit other Kitware open-source projects at
>> http://www.kitware.com/opensource/opensource.html
>>
>> Kitware offers ITK Training Courses, for more information visit:
>> http://kitware.com/products/protraining.html
>>
>> Please keep messages on-topic and check the ITK FAQ at:
>> http://www.itk.org/Wiki/ITK_FAQ
>>
>> Follow this link to subscribe/unsubscribe:
>> http://www.itk.org/mailman/listinfo/insight-developers
>>
> _______________________________________________
> Powered by www.kitware.com
>
> Visit other Kitware open-source projects at
> http://www.kitware.com/opensource/opensource.html
>
> Kitware offers ITK Training Courses, for more information visit:
> http://kitware.com/products/protraining.html
>
> Please keep messages on-topic and check the ITK FAQ at:
> http://www.itk.org/Wiki/ITK_FAQ
>
> Follow this link to subscribe/unsubscribe:
> http://www.itk.org/mailman/listinfo/insight-developers
>