[Debburn-devel] strange memsets

[Thomas Schmitt]

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