[pygame] Bug in sndarray.c

["Eyal Lotem" <eyal.lotem@xxxxxxxxx>]

(Please CC me for all correspondence, thanks)

In pygame 1.7.1release-4.1ubuntu1 (Ubuntu Hardy's version), there is a
bug where 64-bit machines cannot use sndarray_make_sound, as the data
isn't copied, and instead garbage is inserted to the result sound
object.

The function sndarray.c: sndarray_make_sound assumes that the given
arrays' elements sizes will be 1, 2 or 4.  It also unnecessarily casts
the pointers both before and after dereferences.  This unnecessarily
differentiated between otherwise identical handling of the differing
sizes, which causes the need for the cases in the first place.

Perhaps the reason behind the cases was allowing compiler
optimization, but apparently it is too risky (bug-wise).

The "cheap" way out would be to add the case for size=8, but that
would break too upon other word-sized machines.  If this cheap
solution is chosen, its probably a good idea to add a "default" case
which errorizes.  Also, this isn't very optimal, as the switch-case
can be put out of the heavier loop over all the samples (it is
re-checked for every sample, so its not very optimized anyhow!).

The simplifying/fixing patch is attached.

--- pygame-1.7.1release/src/sndarray.c	2004-06-20 07:58:05.000000000 +0300
+++ pygame-1.7.1release/src/sndarray.c.fixed	2008-08-28 05:57:54.000000000 +0300
@@ -191,21 +191,8 @@
             for(loop1=0; loop1<length; loop1++)
             {
                 src = (Uint8*)array->data + loop1*step1;
-                switch(array->descr->elsize)
-                {
-                case 1:
-                    for(loop2=0; loop2<length2; loop2++, dst+=1, src+=step2)
-                        *(Uint8*)dst = (Uint8)*((Uint8*)src);
-                    break;
-                case 2:
-                    for(loop2=0; loop2<length2; loop2++, dst+=1, src+=step2)
-                        *(Uint8*)dst = (Uint8)*((Uint16*)src);
-                    break;
-                case 4:
-                    for(loop2=0; loop2<length2; loop2++, dst+=1, src+=step2)
-                        *(Uint8*)dst = (Uint8)*((Uint32*)src);
-                    break;
-                }
+                for(loop2=0; loop2<length2; loop2++, dst+=1, src+=step2)
+                    *(Uint8*)dst = *(Uint8*)src;
             }
         }
         else
@@ -213,19 +200,15 @@
             for(loop1 = 0; loop1 < length; loop1++)
             {
                 src = (Uint8*)array->data + loop1*step1;
-               switch(array->descr->elsize)
+                switch(step2)
                 {
                 case 1:
                     for(loop2=0; loop2<length2; loop2++, dst+=2, src+=step2)
-                        *(Uint16*)dst = (Uint16)(*((Uint8*)src)<<8);
-                    break;
-                case 2:
-                    for(loop2=0; loop2<length2; loop2++, dst+=2, src+=step2)
-                        *(Uint16*)dst = (Uint16)*((Uint16*)src);
+                        *(Uint16*)dst = (*((Uint8*)src)<<8);
                     break;
-                case 4:
+                default:
                     for(loop2=0; loop2<length2; loop2++, dst+=2, src+=step2)
-                        *(Uint16*)dst = (Uint16)*((Uint32*)src);
+                        *(Uint16*)dst = *(Uint16*)src;
                     break;
                 }
             }