Assign to a null pointer an area inside a function and preserve the value outside

Question

I have a function that reads from a socket, it returns a char** where packets are stored and my intention is to use a NULL unsigned int pointer where I store the length of single packet.

char** readPackets(int numToRead,unsigned int  **lens,int socket){

    char** packets=(char**)malloc(numToRead);
    int *len=(int*)malloc(sizeof(int)*numToRead);

    *(lens)=len;

    for(int i=0;i<numToRead;i++){
        //read
        packets[i]=(char*)malloc(MAX_ETH_LEN);
        register int pack_len=read(socket,packets[i],MAX_ETH_LEN);

        //TODO handler error in  case of freezing

        if(pack_len<=0){
            i--;
            continue;
        }
        len[i]=pack_len;
    }


    return packets;

}

I use it in this way:

unsigned int *lens_out=NULL;
char **packets=readPackets(N_PACK,&lens,sniff_sock[handler]);

where N_PACK is a constant defined previously.

Now the problem is that when I am inside the function everything works, in fact *(lens) points to the same memory area of len and outside the function lens_out points to the same area too. Inside the function len[i] equals to *(lens[i]) (I checked it with gdb).

The problem is that outside the function even if lens_out points to the same area of len elements with same index are different for example

len[0]=46
lens_out[0]=4026546640

Can anyone explain where I made the mistake?

`char** packets=(char**)malloc(numToRead);` is suspicious. I'd expect `.... malloc(sizeof *packets * numToRead);` — chux - Reinstate Monica, Nov 14 '17 at 19:55
i use bytes so sizeof(char)=1 so i think that is uselesse in this case — P.Carlino, Nov 14 '17 at 19:58
`sizeof(char)=1 ` is true but does not apply here. `packets` is a `char **`, not a `char *`. `char** packets = ... malloc(...)` is attempting to allocate memory for a number of pointers, not a number of `char`. — chux - Reinstate Monica, Nov 14 '17 at 19:59
Consider using `malloc(sizeof *pointer *n)` rather than `malloc(sizeof (type_of_pointer)*n)`. It is easier to code correctly (as our comments discussed), review and maintain. — chux - Reinstate Monica, Nov 14 '17 at 20:08
@P.Carlino yes but please don't change the question, it invalidates the comments. Rolled back. — Weather Vane, Nov 14 '17 at 20:08
Post the code that prints `len[0]=46 lens_out[0]=4026546640` — chux - Reinstate Monica, Nov 14 '17 at 20:14
[don't cast malloc()](https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc) — Barmar, Nov 14 '17 at 20:36

score 1 · Accepted Answer · edited Nov 14 '17 at 21:03

Your statement char** packets=(char**)malloc(numToRead) for sure does not reserve enough memory. Note that an element of packets-array is of type char*, and that sizeof(char*) is probably 8 (eventually 4), but very very unlikely 1. So you should write

char** packets = malloc(sizeof(char*) * numToRead)

Otherwise, you write out of the bounds of reserved memory, thereby yielding undefined behaviour (probably the one you explained).

Note further that with i--; continue;, you get memory leaks since you assign a new memory block to the ith element, but you lose reference to the memory reserved right before. Write free(packets[i]);i--;continue; instead.

Further, len[0] is an integral type, whereas lens[0] refers to a pointer to int. Comparing these two does not make sense.

Shouldn't it be `sizeof(char*) * numToRead`? – Barmar Nov 14 '17 at 20:38 — Barmar, Nov 14 '17 at 20:38

score 0 · Answer 2 · answered Nov 14 '17 at 21:16

Firstly, I want to put it out there that you should write clear code for the sake of future maintenance, and not for what you think is optimal (yet). This entire function should merely be replaced with read. That's the crux of my post.

Now the problem is that when I am inside the function everything works

I disagree. On a slightly broader topic, the biggest problem here is that you've posted a question containing code which doesn't compile when copied and pasted unmodified, and the question isn't about the error messages, so we can't answer the question without guessing.

My guess is that you haven't noticed these error messages; you're running a stale binary which we don't have the source code for, we can't reproduce the issue and we can't see the old source code, so we can't help you. It is as valid as any other guess. For example, there's another answer which speculates:

Your statement char** packets=(char**)malloc(numToRead) for sure does not reserve enough memory.

The malloc manual doesn't guarantee that precisely numToRead bytes will be allocated; in fact, allocations to processes tend to be performed in pages and just as the sleep manual doesn't guarantee a precise number of milliseconds/microseconds, it may allocate more or it may allocate less; in the latter case, malloc must return NULL which your code needs to check.

It's extremely common for implementations to seem to behave correctly when a buffer is overflowed anyway. Nonetheless, it'd be best if you fixed that buffer overflow. malloc doesn't know about the type you're allocating; you need to tell it everything about the size, not just the number of elements.

P.S. You probably want select or sleep within your loop, you know, to "handle error in case of freezing" or whatever. Generally, OSes will switch context to another program when you call one of those, and only switch back when there's data ready to process. By calling sleep after sending or receiving, you give the OS a heads up that it needs to perform some I/O. The ability to choose that timing can be beneficial, when you're optimising. Not at the moment, though.

Inside the function len[i] equals to *(lens[i]) (I checked it with gdb).

I'm fairly sure you've misunderstood that. Perhaps gdb is implicitly dereferencing your pointers, for you; that's really irrelevant to C (so don't confuse anything you learn from gdb with anything C-related).

In fact, I strongly recommend learning a little bit less about gdb and a lot more about assert, because the former won't help you document your code for future maintenance from other people, including us, those who you ask questions to, where-as the latter will. If you include assert in your code, you're almost certainly strengthening your question (and code) much more than including gdb into your question would.

The types of len[i] and *(len[i]) are different, and their values are affected by the way types are interpreted. These values can only be considered equal When they're converted to the same type. We can see this through C11/3.19p1 (the definition of "value", where the standard establishes it is dependant upon type). len[i] is an int * value, where-as *(len[i]) is an int value. The two categories of values might have different alignment, representation and... well, they have different semantics entirely. One is for integral data, and the other is a reference to an object or array. You shouldn't be comparing them, no matter how equal they may seem; the information you obtain from such a comparison is virtually useless.

You can't use len[i] in a multiplication expression, for example. They're certainly not equal in that respect. They might compare equal (as a side-effect of comparison introducing implicit conversions), which is useless information for you to have, and that is a different story.

memcmp((int[]){0}, (unsigned char[]){ [sizeof int] = 42 }, sizeof int) may return 0 indicating that they're equal, but you know that array of characters contains an extra byte, right? Yeh... they're equal...

You must check the return value of malloc (and don't cast the return value), if you're using it, though I really think you should reconsider your options with that regard.

The fact that you use malloc means everyone who uses your function must then use free; it's locking down-stream programmers into an anti-pattern that can tear the architecture of software apart. You should separate categories of allocation logic and user interface logic from processing logic.

For example, you use read which gives you the opportunity to choose whatever storage duration you like. This means you have an immense number of optimisation opportunities. It gives you, the downstream programmer, the opportunity to write flexible code which assigns whatever storage duration you like to the memory used. Imagine if, on the other hand, you had to free every return value from every function... That's the mess you're encouraging.

This is especially a poor, inefficient design when constants are involved (i.e. your usecase), because you could just use an automatic array and get rid of the calls to malloc and free altogether... Your downstream programmers code could be:

char packet[size][count];
int n_read = read(fd, packet, size * count);

Perhaps you think using malloc to allocate (and later read) n spaces for packets is faster than using something else to allocate n spaces. You should test that theory, because from my experience computers tend to be optimised for simpler, shorter, more concise logic.

In anticipation:

But I can't return packet; like that!

True. You can't return packet; to your downstream programmer, so you modify an object pointed at by an argument. That doesn't mean you should use malloc, though.

Unfortunately, too many programs are adopting this "use malloc everywhere" mentality. It's reminiscent of the "don't use goto" crap that we've been fed. Rather than listening to cargo cult propaganda, I recommend thinking critically about what you hear, because your peers are in the same position as you; they don't necessarily know what they're talking about.

Assign to a null pointer an area inside a function and preserve the value outside

2 Answers2