buglwIP - A Lightweight TCP/IP stack - Bugs: bug #47172, mem.c mem_malloc implementation...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #47172: mem.c mem_malloc implementation does not properly account for MEM_ALIGNMENT

Submitter:  David Fernandez <david0fc>
Submitted:  Tue 16 Feb 2016 08:51:06 PM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Wont Fix
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Jump to the original submission

Fri 19 Feb 2016 09:06:50 AM UTC, comment #8: 

So your intention is to use a feature for hardware integrity checks to catch programming errors? Sounds a bit odd, but if it works for you, why not. On the other hand, the check is not very safe at it stops checking after using memory once.

Pardon me, but I don't want to add code to lwip for this.

Btw: I do understand the technical reason of the alignment gap. All I said was the C-code is correct - we do not read memory that we don't write before.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 18 Feb 2016 09:34:06 PM UTC, comment #7: 

In some hardwares' memory buses, the alignment gap is also read with the data, then thrown away by the cpu.

If the memory implements parity or ecc checking, you have to write to the alignment gap to set the ecc or parity bits properly.

That "feature" may be used to catch reads to uninitialized memory, but given how the memory bus works, you have to write to the alignment gap, as the writes do use byte enables, for obvious reasons.

I hope that clarifies why we need to write to the alignment gap.

Now zeroizing all the memory is a way forward, but writing to the alignment gap only, provides a way to catch wrong reads, which is nice to have.

Again, not saying lwip is wrong, only saying that if it provides this option, allows some users to avoid doing ugly things to get what they want.

David Fernandez <david0fc>
Thu 18 Feb 2016 08:43:34 PM UTC, comment #6: 

comment #3:

> Sorry, I did not want to imply that you were doing something wrong,


comment #5:

> OK, fine, I thought the preference would be to provide config
> options that avoid doing ugly things, but I guess it is not.


Now I don't know which line I should beleive more...

Anyway, I can't see what's ugly in not initializing memory that's not read (the alignment gap).

We have too many config options to have a readable opt.h and adding more does not make it better. That's the reason I'm reluctant to add yet another option if its value is unclear to me.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 18 Feb 2016 08:31:33 PM UTC, comment #5: 


>> we have to put them somewhere else
>
> So why don't you zero them there?


That is what I'm doing to work around it, but that avoids the extra checking, and thought that adding an optional way to zeroize that metadata in the memory pool or heap shouldn't be a bad thing.

>> The memory block will be zeroed or written first by the user
>> before it tries to read from it
>
> That's optimistic!


I mean, the client of mem_malloc, or memp_malloc will write before reading. That is enforced in our code base by that hardware feature I mentioned.

> I would have bet there are other cases of alignment issues throughout the lwip code like the two you already mentioned.


All the ones I'm interested are the pools and heap, which are just two cases localized to two files, and all that they need is to zeroize with the sizeof the structure, or that size aligned to the MEM_ALIGNMENT (SIZEOF_MEMP_STRUCT)

>> I'm just asking for an additional optional configuration option
>
> Not another config option, please! :-)


OK, fine, I thought the preference would be to provide config options that avoid doing ugly things, but I guess it is not.

David Fernandez <david0fc>
Thu 18 Feb 2016 03:11:38 PM UTC, comment #4: 


> we have to put them somewhere else


So why don't you zero them there?

> The memory block will be zeroed or written first by the user
> before it tries to read from it


That's optimistic! I would have bet there are other cases of alignment issues throughout the lwip code like the two you already mentioned.

> I'm just asking for an additional optional configuration option


Not another config option, please! :-)

Simon Goldschmidt <goldsimon>
Group administrator
Thu 18 Feb 2016 10:51:59 AM UTC, comment #3: 


> I would have thought that both the mem heap and the memp pools go into bss unless you specially tell your linker to put them somewhere else.


Correct, but we have to put them somewhere else, specially the memory pools and heap, are put out of normal data and bss... A platform requirement.

> Also, as the memory block returned is not zeroed, zeroing the struct would not really catch all of these problems.


The memory block will be zeroed or written first by the user before it tries to read from it, but the user does it according to the alignment.

The metadata however, is not that aware of the alignment and byte enables, so doing a memset(mem, 0, sizeof(mem)); will take compiler alignment for that struct into account.

> It seems like this test is not a good one to enforce proper memory usage: we do use the memory correctly, but the platform fails in seeing this. What you see is a false positive. Plus this check only catches the first usage: once the memory was used, written and freed again, the check does nothing after next alloc.


Sorry, I did not want to imply that you were doing something wrong, but rather that our platform needs to take more things into account.

Having it that way catches reads before initialization, which is not a bad thing to have.

I'm just asking for an additional optional configuration option that will make mem and memp zeroize the metadata in the memory pools in a way that accounts for the structure current alignment.

> As such, I'm not convinced this changes helps much.


I agree that is not the usual case, but should not hurt to add something like:

#if LWIP_MEM[P]_FULLY_INITIALIZE_METADATA
  memset(mem, 0, sizeof(mem));
#endif

or

#if LWIP_MEM[P]_FULLY_INITIALIZE_METADATA
  memset(mem, 0, SIZEOF_STRUCT_MEM);
#endif

David Fernandez <david0fc>
Thu 18 Feb 2016 08:28:46 AM UTC, comment #2: 

I would have thought that both the mem heap and the memp pools go into bss unless you specially tell your linker to put them somewhere else.

Also, as the memory block returned is not zeroed, zeroing the struct would not really catch all of these problems.

> so that proper memory usage is enforced


It seems like this test is not a good one to enforce proper memory usage: we do use the memory correctly, but the platform fails in seeing this. What you see is a false positive. Plus this check only catches the first usage: once the memory was used, written and freed again, the check does nothing after next alloc.

As such, I'm not convinced this changes helps much.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 17 Feb 2016 12:14:25 PM UTC, comment #1: 

Note that memp_memory_xxx seems to suffer of a similar problem.


David Fernandez <david0fc>
Tue 16 Feb 2016 08:51:06 PM UTC, original submission:  

The current malloc metadata is defined as:

/**

  • The heap is made up as a list of structs of this type.
  • This does not have to be aligned since for getting its size,
  • we only use the macro SIZEOF_STRUCT_MEM, which automatically aligns.

 */
struct mem {
  /** index (-> ram[next]) of the next struct */
  mem_size_t next;
  /** index (-> ram[prev]) of the previous struct */
  mem_size_t prev;
  /** 1: this area is used; 0: this area is unused */
  u8_t used;
};

Which claims not to need alignment due to the pointers to it being aligned.

The problem is that some platforms require to write the full aligned size, before reads, or they will trigger ECC/Parity errors.

This is specially relied on for heaps (which won't go in bss), so that proper memory usage is enforced.

A simple fix is to memset with 0 the block first, as just initializing the fields, as it is currently done, is not enough (the reason is that some memory systems use byte-enables for writes, but not for reads).

So this bug is to request that a memset is done for the metadata aligned size.

David Fernandez <david0fc>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by david0fc (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-02-18 goldsimon CategoryPlatform ports None
        StatusNone Wont Fix
        Assigned toNone goldsimon
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code