[PATCH v2 4/5] cpukit/libmisc/dumpbuf/dumpbuf.c: Fix undefined behavior for sprintf()
Joel Sherrill
joel.sherrill at oarcorp.com
Thu Sep 3 17:32:30 UTC 2015
On 9/3/2015 12:18 PM, Daniel Gutson wrote:
> On Thu, Sep 3, 2015 at 1:21 PM, Joel Sherrill <joel.sherrill at oarcorp.com> wrote:
>>
>>
>> On 9/2/2015 4:54 PM, Martin Galvan wrote:
>>>
>>> I also used the 'n' versions of the string functions, #define'd magic
>>> numbers
>>> and added a few comments.
>>
>>
>> Great on the 'n' versions!
>>
>> One comment. Why is the output buffer now static? I know it
>> moves it off the stack but what's the consensus on an 80
>> byte array on a stack?
>
> There are some reasons:
> 1) out of the stack, here, means that it will be moved as a static
> memory. If it doesn't fit, the toolchain will warn.
> 2) we don't know how much stack memory is left. This is specially
> important in a debugging function which
> could be called in a faulty context.
> 3) 80 bytes is not that low.
It is a target side utility to print memory in a format similar
to many ROM monitors.
I agree with everything you said and I forget how tiny the RAM
is on some of the new CPUs. I suppose always having it accounted
for is safer than having it as a transient on the stack value.
If you had 512 byte stacks which is feasible for some applications,
you wouldn't want 80 taken by this. But I am also not sure how
much stack calling snprintf() uses either. :)
I'm going to apply this and the other patch. Certainly we
didn't make anything worse.
--joel
>>>
>>> Updates #2405.
>>> ---
>>> cpukit/libmisc/dumpbuf/dumpbuf.c | 121
>>> ++++++++++++++++++++++++---------------
>>> 1 file changed, 75 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/cpukit/libmisc/dumpbuf/dumpbuf.c
>>> b/cpukit/libmisc/dumpbuf/dumpbuf.c
>>> index 9d34d42..bb63997 100644
>>> --- a/cpukit/libmisc/dumpbuf/dumpbuf.c
>>> +++ b/cpukit/libmisc/dumpbuf/dumpbuf.c
>>> @@ -6,7 +6,7 @@
>>> */
>>>
>>> /*
>>> - * COPYRIGHT (c) 1997-2007.
>>> + * COPYRIGHT (c) 1997-2015.
>>> * On-Line Applications Research Corporation (OAR).
>>> *
>>> * The license and distribution terms for this file may in
>>> @@ -24,62 +24,91 @@
>>> #include <rtems/dumpbuf.h>
>>> #include <rtems/bspIo.h>
>>>
>>> +#define HEX_FMT_LENGTH 3 /* Length of the formatted hex string. */
>>> +#define ASCII_FMT_LENGTH 1 /* Length of the formatted ASCII string. */
>>> +#define BYTES_PER_ROW 16 /* Amount of bytes from buffer shown in each
>>> row. */
>>> +#define BARS 2 /* Amount of bars in each row. */
>>> +/* Max length of each row string. */
>>> +#define ROW_LENGTH (BYTES_PER_ROW * (HEX_FMT_LENGTH + ASCII_FMT_LENGTH) +
>>> BARS)
>>> +
>>> /*
>>> * Put the body below rtems_print_buffer so it won't get inlined.
>>> */
>>>
>>> -static inline void Dump_Line(
>>> - const unsigned char *buffer,
>>> - int length
>>> -);
>>> +static void Dump_Line(const unsigned char *buffer, const unsigned int
>>> length);
>>>
>>> -void rtems_print_buffer(
>>> - const unsigned char *buffer,
>>> - int length
>>> -)
>>> +/**
>>> + * @brief Print \p length bytes from \p buffer, both in hex and ASCII.
>>> + * Printing will be done in rows, each showing BYTES_PER_ROW bytes.
>>> + * @details Non-printable chars will appear as dots.
>>> + *
>>> + * @param buffer The buffer we'll print.
>>> + * @param length Amount of bytes from \p buffer we'll print. This can't
>>> be
>>> + * unsigned because we don't have a way to check if we're erroneously
>>> getting
>>> + * a negative \p length.
>>> + */
>>> +void rtems_print_buffer(const unsigned char *buffer, const int length)
>>> {
>>> + unsigned int i, mod, max;
>>>
>>> - int i, mod, max;
>>> -
>>> - if ( !length ) return;
>>> + if (length > 0) {
>>> + mod = length % BYTES_PER_ROW;
>>>
>>> - mod = length % 16;
>>> + max = length - mod;
>>>
>>> - max = length - mod;
>>> + /* Print length / BYTES_PER_ROW rows. */
>>> + for (i = 0; i < max; i += BYTES_PER_ROW) {
>>> + Dump_Line(&buffer[i], BYTES_PER_ROW);
>>> + }
>>>
>>> - for ( i=0 ; i<max ; i+=16 )
>>> - Dump_Line( &buffer[ i ], 16 );
>>> -
>>> - if ( mod )
>>> - Dump_Line( &buffer[ max ], mod );
>>> + /* Print another row with the remaining bytes. */
>>> + if (mod > 0) {
>>> + Dump_Line(&buffer[max], mod);
>>> + }
>>> + } else {
>>> + printk("Error: length must be greater than zero.");
>>> + }
>>> }
>>>
>>> -static inline void Dump_Line(
>>> - const unsigned char *buffer,
>>> - int length
>>> -)
>>> +/**
>>> + * @brief Print \p length bytes from \p buffer, both in hex and ASCII.
>>> + * @details Non-printable chars will appear as dots.
>>> + *
>>> + * @param buffer The buffer we'll print.
>>> + * @param length Amount of bytes from \p buffer we'll print.
>>> + */
>>> +static void Dump_Line(const unsigned char *buffer, const unsigned int
>>> length)
>>> {
>>> -
>>> - int i;
>>> - char line_buffer[120];
>>> -
>>> - line_buffer[0] = '\0';
>>> -
>>> - for( i=0 ; i<length ; i++ )
>>> - sprintf( line_buffer, "%s%02x ", line_buffer, buffer[ i ] );
>>> -
>>> - for( ; i<16 ; i++ )
>>> - strcat( line_buffer, " " );
>>> -
>>> - strcat( line_buffer, "|" );
>>> - for( i=0 ; i<length ; i++ )
>>> - sprintf( line_buffer, "%s%c", line_buffer,
>>> - isprint( buffer[ i ] ) ? buffer[ i ] : '.' );
>>> -
>>> - for( ; i<16 ; i++ )
>>> - strcat( line_buffer, " " );
>>> -
>>> - strcat( line_buffer, "|\n" );
>>> -
>>> - printk( line_buffer );
>>> + unsigned int i;
>>> + static unsigned char line_buffer[ROW_LENGTH] = "";
>>> + size_t tmp_len;
>>> +
>>> + /* Output the hex value of each byte. */
>>> + for (i = 0; i < length; ++i) {
>>> + snprintf(&line_buffer[i * HEX_FMT_LENGTH], HEX_FMT_LENGTH + 1,
>>> + "%02x ", buffer[i]);
>>> + }
>>> +
>>> + /* Fill the remaining space with whitespace (if necessary). */
>>> + for (; i < BYTES_PER_ROW; ++i) {
>>> + strncat(line_buffer, " ", HEX_FMT_LENGTH);
>>> + }
>>> +
>>> + /* Append a bar. */
>>> + strncat(line_buffer, "|", 1);
>>> + tmp_len = strnlen(line_buffer, ROW_LENGTH);
>>> +
>>> + /* Now output the ASCII glyphs of printable chars. */
>>> + for (i = 0; i < length; ++i) {
>>> + snprintf(&line_buffer[tmp_len + i], ASCII_FMT_LENGTH + 1,
>>> + "%c", isprint(buffer[i]) ? buffer[i] : '.');
>>> + }
>>> +
>>> + /* Fill the remaining space with whitespace (if necessary). */
>>> + for(; i < BYTES_PER_ROW; i++) {
>>> + strncat(line_buffer, " ", ASCII_FMT_LENGTH);
>>> + }
>>> +
>>> + /* Append another bar and print the resulting string. */
>>> + printk("%s|\n", line_buffer);
>>> }
>>>
>>
>> --
>> Joel Sherrill, Ph.D. Director of Research & Development
>> joel.sherrill at OARcorp.com On-Line Applications Research
>> Ask me about RTEMS: a free RTOS Huntsville AL 35805
>> Support Available (256) 722-9985
>>
>> _______________________________________________
>> devel mailing list
>> devel at rtems.org
>> http://lists.rtems.org/mailman/listinfo/devel
>
>
>
--
Joel Sherrill, Ph.D. Director of Research & Development
joel.sherrill at OARcorp.com On-Line Applications Research
Ask me about RTEMS: a free RTOS Huntsville AL 35805
Support Available (256) 722-9985
More information about the devel
mailing list