Bad free() in WaveWeakContainer.m

[orinem]

(_data[iv[2]])[iv[1]] is set to obj and promptly freed.

(_data[iv[2]])[iv[1]] is used a few lines later.

The malloc and memset sizes seem strange. I suggest the following.

I also suggest replacing a memcpy of two bytes with the direct assignments.

diff --git a/Sources/Core/WaveWeakContainer.m b/Sources/Core/WaveWeakContainer.m
index fc2ac3b..8a5c15c 100644
--- a/Sources/Core/WaveWeakContainer.m
+++ b/Sources/Core/WaveWeakContainer.m
@@ -55,14 +55,14 @@
 - (void)setBytes:(const UInt8*)bytes forIV:(const UInt8*)iv {
     UInt8 *d;
     if (_data[iv[2]] == nil) {
-        _data[iv[2]] = malloc(sizeof(_data)+sizeof(iv[2]));
+        _data[iv[2]] = malloc(sizeof(_data));
         NSAssert(_data[iv[2]], @"malloc failed");
         memset(_data[iv[2]], 0, sizeof(_data));
     }

     if ((_data[iv[2]])[iv[1]] == nil)
        {
-               void *obj = malloc(sizeof(_data) * sizeof(iv[1]));
+               void *obj = malloc(3*sizeof(UInt8)*LAST_BIT);
                if (obj == NULL) {
                        DBNSLog(@"malloc failed");
                        return;
@@ -70,13 +70,15 @@
                else
                {
                        (_data[iv[2]])[iv[1]] = obj;
-                       memset((_data[iv[2]])[iv[1]], 0, sizeof(obj));
-                       free(obj);
+                       memset((_data[iv[2]])[iv[1]], 0, 3*sizeof(UInt8)*LAST_BIT);
+                       //free(obj);
                }
     }

     d = &((_data[iv[2]])[iv[1]])[iv[0] * 3];
-    memcpy(d + 1, bytes, 2);
+    d[1] = bytes[0];
+    d[2] = bytes[1];
+    //memcpy(d + 1, bytes, 2);

     if (d[0] == 0) {
         d[0] = 1;

[ikorich]

_data[iv[2]] = malloc(sizeof(data)): Result of 'malloc' is converted to a pointer of type 'UInt8 ', which is incompatible with sizeof operand type 'UInt8 *_[256]'

memset((_data[iv[2]])[iv[1]], 0, 3_sizeof(UInt8)_LAST_BIT): Potential leak of memory pointed to by 'obj'

[orinem]

I didn't get those errors, but I was wondering about it...

_data is UInt8 [256], so _data[iv[2]] is UInt8 - OK, I see where the warning comes from and I suppose the sizeof(iv[2]) avoided it.

It should really be malloc(sizeof(UInt8 )LAST_BIT) to get an array of 256 Uint8 *.

There is no memory leak of obj, it is freed in dealloc: free((_data[x])[y]), but even eliminating 'obj' doesn't fix the false positive.

Treating malloc failures the same and cleaning up the mix of nil/NULL, I think it should be:

- (void)setBytes:(const UInt8*)bytes forIV:(const UInt8*)iv
{
    UInt8 *d;
    if (_data[iv[2]] == NULL)
    {
        _data[iv[2]] = malloc(sizeof(UInt8 *)*LAST_BIT);
        if(_data[iv[2]] == NULL)
        {
            DBNSLog(@"malloc failed");
            return;
        }
        memset(_data[iv[2]], 0, sizeof(UInt8 *)*LAST_BIT);
    }

    if ((_data[iv[2]])[iv[1]] == NULL)
    {
        (_data[iv[2]])[iv[1]] = malloc(3*sizeof(UInt8)*LAST_BIT);
        if ((_data[iv[2]])[iv[1]] == NULL) {
            DBNSLog(@"malloc failed");
            return;
        }
        memset((_data[iv[2]])[iv[1]], 0, 3*sizeof(UInt8)*LAST_BIT);
    }

    d = &((_data[iv[2]])[iv[1]])[iv[0] * 3];
    // no point setting up memcpy for 2 bytes
    d[1] = bytes[0];
    d[2] = bytes[1];

    if (d[0] == 0)
    {
        d[0] = 1;
        ++_count;
    } 
}

[ikorich]

menu Product -> Analyze

[orinem]

Right. I made my last comment after running Analyze.

It fixed the incompatible size warning, but still had the false positive "Potential memory leak".

As far as I can see, the data structure is implementing a sparse version of

UInt8 _data[256][256][256][3];

indexed as follows:

UInt8 iv0 = iv[0], iv1 = iv[1], iv2 = iv[2]; // for readability
UInt8 *d = _data[iv2][iv1][iv0];
d[0] = 1; // once only
d[1] = bytes[0];
d[2] = bytes[1];

The actual indexing:

    d = &((_data[iv[2]])[iv[1]])[iv[0] * 3];

implies that the data structure is actually:

UInt8 _data[256][256][256*3];

which is what my latest code implemented.

[ikorich]

could you make pull request?

[orinem]

I will do it tonight - the code is at home.

[orinem]

Fixed: https://github.com/IGRSoft/KisMac2/commit/73ce3b08ff042a9befdfb93b4c94c150f59a0db0