[Debburn-devel] strange memsets

[Frantisek Kluknavsky]

Thank you. I will use the magic constant 16.

#include <stdio.h>
void f(unsigned char arr[23])
{
         printf("parameter: %ld %ld\n", sizeof(arr), sizeof(*arr));
}
int main()
{
         unsigned char arr[23];
         printf("local array: %ld %ld\n", sizeof(arr), sizeof(*arr));
         f(arr);
         return 0;
}

Output with gcc-4.7.2 64-bit is
local array: 23 1
parameter: 8 1


On 02/23/2013 05:10 PM, Thomas Schmitt wrote:
> Hi,
>
> my mail did not show up in the list archive since yesterday.
> So i resend an updated version.
>
> --------------------------------------------------------------------
>
> I am not involved in genisoimage, but in libjte which stems from
> the JTE code in genisoimage.
>
> So i comment on both code bases, refering to
>    http://anonscm.debian.org/viewvc/debburn/cdrkit/trunk/genisoimage/
> (last change: Thu Apr 22 23:32:47 2010 UTC)
>
> --------------------------------------------------------------------
>
>> +++ wrk/genisoimage/jte.c    2013-02-22 13:31:50.967667591 +0100
>> @@ -280,7 +280,7 @@ extern int list_file_in_jigdo(char *file
>>
>> -    memset(md5, 0, sizeof(md5));
>> +    memset(md5, 0, sizeof(*md5));
> This change seems wrong, because the variable md5 is declared by
>
>    extern int list_file_in_jigdo( ... , unsigned char md5[16])
>
> The statement shall zero all 16 bytes of md5, not only one byte as
> would happen with  sizeof(*md5)  ( == sizeof(unsigned char)).
>
> If the compiler really sees sizeof(md5) == 4 or 8, then one should
> explicitely state 16 instead of sizeof(md5).
>
> Could you make some test with e.g.
>      printf("sizeof(md5)= %d\n", (int) sizeof(md5));
> I get
>      sizeof(md5)= 16
>
>
> --------------------------------------------------------------------
>
>> +++ wrk/genisoimage/md5.c    2013-02-22 13:15:29.979935133 +0100
>> @@ -183,7 +183,7 @@ mk_MD5Final (unsigned char digest[16], s
>> -    memset(ctx, 0, sizeof(ctx));    /* In case it's sensitive */
>> +    memset(ctx, 0, sizeof(*ctx));    /* In case it's sensitive */
> This change seems indeed valid, because we have as declaration
>    void
>    mk_MD5Final (unsigned char digest[16], struct mk_MD5Context *ctx)
>
> The original statement would only zeroize 4 or 8 bytes.
> Memory-wise harmless, because sizeof(struct mk_MD5Context) >= 70.
> But it does not fulfill its job properly.
>
> I will change this in my local copy of libjte which goes into
> GNU xorriso.
>
> Oh my ... where do i have the test code for Jigdo production ... ?
>
> Update: I meanwhile ran under valgrind a test that repacks
> debian-testing-i386-netinst.iso as Jigdo files. 0 errors found.
>
> The change seems safe. I recommend to apply it to libjte.
>
>
> --------------------------------------------------------------------
>
>> +++ wrk/libedc/edcspeed.c    2013-02-22 10:46:52.497686387 +0100
>> @@ -35,7 +35,8 @@ static int encspeed()
>> -    memset(sect, 0, sizeof(sect));
>> +    //unneccessary, immediately rewritten again
>> +    //memset(sect, 0, sizeof(sect));
> Well, that's outside of JTE code. Not my sandbox.
>
> But i would say this is not only unneccessary but also wrong.
> One should probably remove that line without leaving a comment.
> (Does genisoimage coding style allow C++ comments ?)
>
>
> --------------------------------------------------------------------
>
> Have a nice day :)
>
> Thomas
>