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.

The actual value of size is `7654`. And the size of the image is trully 7654 bytes. No, of course it is not `-1` Why would it be `-1`? If that was true.. 47 bytes allocated has to be enough. — Edenia, Aug 09 '14 at 22:58

score 3 · Answer 1 · answered Aug 09 '14 at 23:09

3

Your H loop is wrong. If your image is 100 pixels high, your loop from 101 to 1 rather than 99 to 0. As a consequence, bmp_find_xy calculates the wrong index.

Try replacing the loop with:

H = h;
while (h-- > 0) {

This pattern is sometimes jokingly called the "goes to operator". It is a safe way to decrement an unsigned variable in a loop towards zero.

answered Aug 09 '14 at 23:09

1000 Bites

1,010
9
9

Worth noting, the "goes to" operator reference comes from the syntax when typing the expression without spaces: `var-->0`. Note the "arrow" pointing the var to the target value. +1 for mentioning it. [See this](http://stackoverflow.com/questions/1642028/what-is-the-name-of-this-operator). – WhozCraig Aug 09 '14 at 23:33

WhozCraig · Accepted Answer · 2014-08-09T23:29:50.897

2

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.

edited Aug 09 '14 at 23:29

answered Aug 09 '14 at 23:08

WhozCraig

65,258
11
75
141

@Edenia glad to help. it is one of the few places where post-decrement/increment make sense to use. Spend a little time on it and it should be clear. – WhozCraig Aug 09 '14 at 23:15
Yes.. and the `while` method is nice because it will decrement H until it reaches false i.e 0. Doing it with `while` looks better. – Edenia Aug 09 '14 at 23:20
@Edenia the assessment of whether to continue or not is made with the value *prior* to the decrement. that is important. It means you could have an unsigned `H` that was `0` and the loop would simply be skipped. Such logic isn't nearly as clean (in fact requiring unusual decrementing logic or a special-case if-check) when using a for-loop (which I will add to the answer so you can compare in a moment). – WhozCraig Aug 09 '14 at 23:22
For clarity, I'd prefer `for (H=0; H – Jongware Aug 09 '14 at 23:33
@WhozCraig such declaration like `for (dword var=h; var--;)` won't work on mine compiler. It won't even work for gcc.. (ANSI-C) i thought thats known to be C++-like cast. – Edenia Aug 10 '14 at 00:27
It's c99 or later. Add -std=c99 and try again. – WhozCraig Aug 10 '14 at 02:24

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

2 Answers2