Finding the first occurance of a specific color - weird allocation behaviour

Question

I am writing a function that returns the address of the first located pixel with specified color. Right now i am testing the detection. Currently on 50x50 bmp. Considering this code:

dword bmp_find_xy (dword xp, dword yp)                                          
{                                                                                                
    dword w = 50;
    word bpx = (3*8);
    dword offset = (2+sizeof(BMP)+sizeof(DIB));
    dword row = (((bpx * w) * 4) / 32);
    dword pixAddress = (offset) + row * yp + ((xp * bpx) / 8);

    return pixAddress;
}                                                                                                

dword bmp_dfind_c (char *FILE_NAME, BYTE R, BYTE G, BYTE B)
{
    dword w = 50;
    dword h = 50;
    dword size;

    int W, H, i;

    FILE* fp = fopen("sample.bmp", "r+b");
    BYTE* bmp;

    fseek(fp, 0L, SEEK_END);
    size = ftell(fp);
    bmp = malloc(size+48); // note this line
    rewind(fp);

    for(i=0; i<size; i++)
    bmp[i] = fgetc(fp);

    fseek(fp, 54, SEEK_SET);

    for(H = h; H >=1; H--)
    {
        for(W = 0; W < w; W++)
        {
            if(bmp[bmp_find_xy(FILE_NAME, W, H)] == 255)
            printf("There is a pix with maxed value");
        }
    }
    fclose(fp);
    return 1;
}

So.. since im using ancient compiler and there are no optimizations.. im receiving error for buffer overflow if i don't put at least +48 to size. Why 48? Why it overflows if i put only malloc(size) that makes no sense to me.

Answer 1

Pardon me if I question your H-decrement logic. Assuming your H and W are zero-based (and they should be), I believe this:

for(H = h; H >=1; H--)

would be far better served as this:

H = h;
while (H--)

if it is really the intend of backward-enumerator H-1 down to 0. (and I think it should be). Fix that I and I'm fairly sure you can properly allocate your buffer. As written, the expectation is you're searching in the Hth row, to start your loop, and such a row is not in your buffer.

If a for-loop is mandatory, it actually involves more work. Something as seemingly innocent as this:

for(H=(h-1); H>=0; --H)

will not work as a universal mechanism. What happens when h, and unsigned type, is 0 to start with?. The expression (h-1) would introduce an underflow. Ouch. Certainly you could "fix" this by doing something like:

if (h)
{
    for(H=(h-1); H>=0; --H)
    ....
}

but that is just piling more cruft on what was originally a bad idea.

The following will work, but is considerably less clear imho:

for (H=h; H--;)

this accomplishes the same logic as our while-loop, at the cost of obfuscation. The one (and only) thing it brings to the table is local-var-declaration, which can be helpful:

for (dword var=h; var--;)

There are many ways to do this, these but a few. I find the first the clearest in your context. Best of luck.