change log for rtems (2011-03-15)

[Sebastian Huber]

On 03/15/2011 04:27 PM, Ralf Corsepius wrote:
> On 03/15/2011 04:10 PM, rtems-vc at rtems.org wrote:
> 
>> 2011-03-15    Joel Sherrill<joel.sherrill at oarcorp.com>
>>
>>     * libmisc/shell/main_setenv.c: Address Coverity issue 134 which
>>     recommends using strnXXX methods.
>>
>> M 1.2778  cpukit/ChangeLog
>> M    1.4  cpukit/libmisc/shell/main_setenv.c
>>
>> diff -u rtems/cpukit/ChangeLog:1.2777 rtems/cpukit/ChangeLog:1.2778
>> --- rtems/cpukit/ChangeLog:1.2777    Tue Mar 15 09:52:31 2011
>> +++ rtems/cpukit/ChangeLog    Tue Mar 15 10:02:41 2011
>> @@ -1,3 +1,8 @@
>> +2011-03-15    Joel Sherrill<joel.sherrill at oarcorp.com>
>> +
>> +    * libmisc/shell/main_setenv.c: Address Coverity issue 134 which
>> +    recommends using strnXXX methods.
>> +
>>   2011-03-15    Sebastian Huber<sebastian.huber at embedded-brains.de>
>>
>>       * libmisc/cpuuse/cpuusagereport.c: Avoid assumptions on execution
>>
>> diff -u rtems/cpukit/libmisc/shell/main_setenv.c:1.3
>> rtems/cpukit/libmisc/shell/main_setenv.c:1.4
>> --- rtems/cpukit/libmisc/shell/main_setenv.c:1.3    Sat Aug 28
>> 15:10:00 2010
>> +++ rtems/cpukit/libmisc/shell/main_setenv.c    Tue Mar 15 10:02:41 2011
>> @@ -16,6 +16,13 @@
>>   #include<rtems/shell.h>
>>   #include "internal.h"
>>
>> +/*
>> + *  Limit examining or copying more than 256 characters at a time.
>> + *  Yes, this is very arbitrary.  If there are POSIX constants, then
>> + *  they should be used.
>> + */
>> +#define MAX 156
>> +
>>   int rtems_shell_main_setenv(int argc, char *argv[])
>>   {
>>     char* env = NULL;
>> @@ -32,7 +39,7 @@
>>     env = argv[1];
>>
>>     for (arg = 2; arg<  argc; arg++)
>> -    len += strlen(argv[arg]);
>> +    len += strnlen(argv[arg], MAX);
>>
>>     len += argc - 2 - 1;
>>
>> @@ -44,8 +51,8 @@
>>     }
>>
>>     for (arg = 2, p = string; arg<  argc; arg++) {
>> -    strcpy(p, argv[arg]);
>> -    p += strlen(argv[arg]);
>> +    strncpy(p, argv[arg], MAX);
>> +    p += strnlen(argv[arg], MAX);
>>       if (arg<  (argc - 1)) {
>>         *p = ' ';
>>         p++;
>>
> 
> argv is defined as being 0 terminated
> => using strnlen is wrong.
> => Coverity is wrong

Yes, the strlen() to strnlen() change is nonsense.  Coverity doesn't like
strcpy() in general.  It is happy if you use strncpy() or memcpy() instead so
it can check the length parameter.  Since we know the capacity of "string" we
could pass a worst case length, but this makes all more complicated for
nothing.  I would revert the change and mark it in Coverity as Intentional.

> 
> Also, by limiting the string you are
> * causing bugs
> * introducing memory bloat (a static buffer).
> 
> IMO this patch is simply plain wrong and should be reverted.
> 
> _______________________________________________
> rtems-vc mailing list
> rtems-vc at rtems.org
> http://www.rtems.org/mailman/listinfo/rtems-vc


-- 
Sebastian Huber, embedded brains GmbH

Address : Obere Lagerstr. 30, D-82178 Puchheim, Germany
Phone   : +49 89 18 90 80 79-6
Fax     : +49 89 18 90 80 79-9
E-Mail  : sebastian.huber at embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.