WARP Project Forums - Wireless Open-Access Research Platform

You are not logged in.

#1 2016-Jul-15 09:56:00

kirchhof
Member
From: Aachen
Registered: 2016-Jul-13
Posts: 4

Comparison of Pointer and Int in wlan_mac_sta.c

Hi,

I'm trying to understand the STA code from the 802.11 Ref. Des. (wlan_mac_sta.c)
In line 696 it says:

Code:

wlan_addr_eq(attempt_bss_info && curr_bss_info->bssid, attempt_bss_info->bssid)

To the best of my knowledge (attempt_bss_info && curr_bss_info->bssid), i.e. (pointer && pointer), should be of type Integer and, because it is the logical && operator, be either 0 or 1.

The wlan_addr_eq is defined in wlan_mac_common.h:

Code:

#define wlan_addr_eq(addr1, addr2)  (memcmp((void*)(addr1), (void*)(addr2), 6)==0)

This code to me seems to compare two MAC addresses for equality.
Converting Integer to void* should throw a warning and I don't think that 0 or 1 points to anything useful for the comparison.

But maybe I overlooked something...
Could you please tell me if this is an error or not? And if it's not an error, could you please explain me, what this code is supposed to do?

Thank you in advance for your help :-)
Have a nice weekend!

Offline

 

#2 2016-Jul-17 10:58:26

chunter
Administrator
From: Mango Communications
Registered: 2006-Aug-24
Posts: 1052

Re: Comparison of Pointer and Int in wlan_mac_sta.c

That's definitely an error. Sorry about that. I suspect the origin of that error was a careless copy and paste. Line 696 should be

Code:

if(attempt_bss_info && wlan_addr_eq(curr_bss_info->bssid, attempt_bss_info->bssid)) wlan_mac_sta_join_return_to_idle();

The "attempt_bss_info &&" should be outside the "wlan_addr_eq()" macro. It is intended to be a short-circuit evaluation to prevent "attempt_bss_info->bssid" from being called in the event that attempt_bss_info is NULL.

Thanks for pointing that out. We'll fix it officially in the next release.

Offline

 

#3 2016-Jul-18 03:22:04

kirchhof
Member
From: Aachen
Registered: 2016-Jul-13
Posts: 4

Re: Comparison of Pointer and Int in wlan_mac_sta.c

Thank you for your fast response. But I'm somewhat confused by your comment on null pointer dereferenciation.

chunter wrote:

to prevent "attempt_bss_info->bssid" from being called in the event that attempt_bss_info is NULL.

Up to now, I actually believed that you are OK with dereferencing null pointers (to avoid the additional "!= 0" check for performance reasons) as long as you do not expect meaningful results from the operation.

For example in line 963 of wlan_mac_station_info.c, curr_station_info_entry should be NULL once you reached the last list element and try to access the next. But since you're never making use of station_info_temp again but just leave the while loop, I thought this is OK for you.

Or to give you a different less obvious example: In wlan_mac_sta.c you dereference active_bss_info in line 1025. I'm not that deep in the sta code that I would be able to guarantee for that, but to my understanding you're only setting active_bss_info inside of configure_bss() and only if the parameter (bss_config) is non-null. Under this assumption, active_bss_info would be null in line 1025 the first time you're calling configure_bss() with a non-null parameter (which I think should be in wlan_mac_sta_join.c's wlan_mac_sta_join_bss_attempt_poll method where you're also setting the BSS_FIELD_MASK_CHAN flag that leads you to line 1025).
But I'm almost sure, that I overlooked some different place in code where you're setting active_bss_info before it comes to that case. I'm not saying your code is erroneous there; I only noticed that you're checking active_bss_info for NULL equality several times in configure_bss() and was confused by why you didn't test for it in line 1025 and then checked who calls configure_bss() ;-) So most probably I think you just left out the non-null check for performance reasons or because you're sure the case I described never happens because active_bss_info gets set somewhere else.

And please let me thank you again for your super-fast support here in the forum! I really appreciate it!
Have a nice day :-)

Offline

 

#4 2016-Jul-18 10:11:49

chunter
Administrator
From: Mango Communications
Registered: 2006-Aug-24
Posts: 1052

Re: Comparison of Pointer and Int in wlan_mac_sta.c

kirchhof wrote:

Up to now, I actually believed that you are OK with dereferencing null pointers (to avoid the additional "!= 0" check for performance reasons) as long as you do not expect meaningful results from the operation.

The short answer is that de-referencing a null pointer leads to undefined behavior. I suspect that it's okay as long as:

(a) Like you said, you don't expect meaningful results

and

(b) That you don't attempt to write anything to the de-referenced NULL pointer.


kirchhof wrote:

For example in line 963 of wlan_mac_station_info.c, curr_station_info_entry should be NULL once you reached the last list element and try to access the next. But since you're never making use of station_info_temp again but just leave the while loop, I thought this is OK for you.

That looks like another error, but I agree that this one is harmless (I'll still fix it in the next release). You can just remove line 963 entirely since station_info_temp is assigned at the top of the loop. I think this is what is happening in the code as it is currently written:

1. curr_station_info_entry evaluates to NULL on the final loop. In other words, curr_station_info_entry = 0x00000000.
2. 0x00000000 gets casted as a dl_entry. curr_station_info_entry->data evaluated as 0x00000008, since "data" is 8 bytes into the dl_entry struct definition.
3. 0x00000008 gets casted as a station_info_t* in the assignment of station_info_temp. In this code, station_info_temp does not then get de-referenced (which is good). If it did, whatever bytes happen to live in 0x00000008 would be as the address of the station_info_t. The bytes in 0x00000008 are from the sw_exception vectors

Because we didn't actually do anything with station_info_temp, this was safe. However, it's definitely dangerous as it would be pretty easy to start manipulating fields as if that were a valid station_info_t. Because this is a baremetal C program without an OS to help us, doing this wouldn't lead to any kind of helpful segfault. Instead, we would be blindly overwriting important memory addresses with junk data. These kinds of problems are exceptionally annoying to debug since the node can hard crash or even reset itself (the reset vector lives at 0x00000000).

kirchhof wrote:

Or to give you a different less obvious example: In wlan_mac_sta.c you dereference active_bss_info in line 1025. I'm not that deep in the sta code that I would be able to guarantee for that, but to my understanding you're only setting active_bss_info inside of configure_bss() and only if the parameter (bss_config) is non-null. Under this assumption, active_bss_info would be null in line 1025 the first time you're calling configure_bss() with a non-null parameter (which I think should be in wlan_mac_sta_join.c's wlan_mac_sta_join_bss_attempt_poll method where you're also setting the BSS_FIELD_MASK_CHAN flag that leads you to line 1025).
But I'm almost sure, that I overlooked some different place in code where you're setting active_bss_info before it comes to that case. I'm not saying your code is erroneous there; I only noticed that you're checking active_bss_info for NULL equality several times in configure_bss() and was confused by why you didn't test for it in line 1025 and then checked who calls configure_bss() ;-) So most probably I think you just left out the non-null check for performance reasons or because you're sure the case I described never happens because active_bss_info gets set somewhere else.

You are finding a lot of my bugs -- apologies and thanks.  It looks like I'm responsible for creating that bug just before the release of v1.5.0 (back before we renamed "my_bss_info" to "active_bss_info"). Line 1025 should be:

Code:

if(wlan_verify_channel(wlan_mac_high_bss_channel_spec_to_radio_chan(bss_config->chan_spec)) != XST_SUCCESS) {

(i.e. swap "bss_config" for "active_bss_info"). That code is supposed to sanity check the channel specified in bss_config (which must be non-NULL due to the check in line 995). If that the bss_config specifies an invalid channel, configure_bss() is supposed to quit and return BSS_CONFIG_FAILURE_CHANNEL_INVALID before altering anything in active_bss_info. I'm surprised the code as written even works -- I think we got really lucky. When active_bss_info is NULL when configure_bss() is called, active_bss_info->chan_spec casts 0x00000006 as a chan_spec_t* and proceeds to de-reference it. So, whatever bytes are in 0x00000006 are treated as a pointer to a chan_spec_t. Apparently, by dumb luck, the evaluation of wlan_mac_high_bss_channel_spec_to_radio_chan() on those bytes happens to yield XST_SUCCESS and configure_bss() is allowed to move on. In the steady state after the STA has joined, it is then safe to call configure_bss() since active_bss_info will be non-NULL.

In general, there shouldn't be any place in the code where we de-reference a NULL pointer intentionally (though you have just found 3 places where we did so unintentionally). As far as building a reference design that tries to maximize usability, I think this is a good design goal for us. If NULL checking is too expensive an operation for your application then you can choose to trade speed for safety, but definitely keep on top of your uses of that strategy -- I know from experience that debugging NULL pointer problems is very difficult.

Offline

 

Board footer