Fix use of uint32 to store a memory address

[skliper]

In many places a uint32 is used to store a memory address which breaks horribly on 64-bit architectures.

The new version of OSAL "common_types.h" introduced a "cpuaddr" type to address this -- it is defined as an integer type large enough to store a memory address on the local processor.

This ticket is to replace all uses of a uint32 to store a memory address with the cpuaddr type for better portability. This is a requirement for a native 64-bit build to work.

[skliper]

Imported from trac issue 7. Created by jphickey on 2014-12-24T08:57:49, last modified: 2019-03-05T14:57:55

[skliper]

Trac comment by jphickey on 2014-12-24 09:01:39:

Note that this ticket cannot fix the issue of putting addresses //in messages// yet. Putting a memory address directly within a CFE message is a bad idea for numerous reasons, particularly if the message gets mangled it will likely crash the process. But another issue is that on some CPUs the field needs to be 64 bits and on other CPUs it needs to be 32 bits.

The "electronic data sheets" (EDS) files can handle this discrepancy, but for now the messages will be left as-is. At some point in the future we should consider replacing memory address with a more abstract identifier that can be validated before use.

[skliper]

Trac comment by glimes on 2014-12-26 12:13:23:

I'd like to see us review the code involved with an eye toward actually declaring the storage as a pointer -- and review any messages containing pointers to make sure the value is actually useful, and see if maybe a small integer index can be substituted.

[skliper]

Trac comment by jphickey on 2014-12-26 21:20:17:

Good point -- Here is the general rule of thumb I //like// to use and what I would propose going forward, if we had a detailed coding standard:

• Pointers to blocks of memory used for unspecified/generic data storage are "uint8 *". The reason to use uint8 here is so that integer offsets can be easily added.
• Any other opaque pointers to objects should be "void *", such as a library call that takes a generic object argument.
• Any other "memory address" that //cannot// be dereferenced by the local CPU should be an unsigned integer value (e.g. uintptr_t from stdint.h). Example being //physical// memory addresses on a cpu with an MMU, or anything that hasn't been mapped into the memory space. The problem with making these values as pointers is that someone might be tempted to dereference it.

The key on the first two points is that pointers are for memory that is valid to this process, it is just that the specific usage is not known, vs. integers where the memory address is not valid to this process.

Also a side note on the last point -- in general we should also stop passing direct memory addresses outside the current process/app. This means removal of memory addresses from external messages altogether since this is inherently unsafe. Many, many system crashes I've had to debug over the years due to "believing" a memory address passed in a message and causing a segfault.... The better way to do this is to encode the value into an integer (such as buffer # and offset) that can be checked before use at the receiver.

With all that being said, the code changes to make the uint32's into pointers may be more significant than just simply changing to a proper length unsigned integer. The reason is that a lot of integer operations are performed on the addresses, such as addition/subtraction of offsets and bitwise "AND" for checking alignment. These tend to break when given an actual pointer. (A uint8 pointer may work in most of the cases since addition and subtraction will work as expected at least).

[skliper]

Trac comment by jphickey on 2014-12-26 21:27:52:

My proposal going forward here is to first change all the usage of uint32 as address to be cpuaddr/uintptr type, as this has a pretty low risk of breaking anything. In fact on a regular 32 bit build it will be identical because the cpuaddr/uintptr type will actually be a uint32. I can also confirm that after making this change and building/running on a 64-bit CPU, things also work as expected, so this is the fastest/safest way to make a 64 bit build work for real.

As a future step, these can be converted to actual pointers where appropriate.

[skliper]

Trac comment by jphickey on 2015-01-05 17:08:55:

Pushed the first step of this change to branch "trac-7-uint32_to_cpuaddr", commit [changeset:d8f0b8d].

This changes the uint32 type to "cpuaddr" where it is holding a memory address. Some of these very well should be pointers instead (void* or uint8*) but keeping them as an integer type for now will reduce the chance of unintended side effects.

Branch name is "trac-7-uint32_to_cpuaddr"

[skliper]

Trac comment by jphickey on 2015-04-06 11:17:47:

This is ready for review/merge

[skliper]

Trac comment by acudmore on 2015-04-06 14:12:45:

Recommend accept.

We have to make sure the cFE, PSP, and OSAL are all in sync for this, correct?

[skliper]

Trac comment by jphickey on 2015-04-06 14:19:11:

In order to build and actually run on a 64-bit system, the answer is yes, all components need to have the uint32 -> cpuaddr fix applied before it will work.

However, on a 32-bit build, the cpuaddr and uint32 types are equivalent so this is convenient for migration purposes -- should still build and run the same even if not all components are fixed yet.

[skliper]

Trac comment by jwilmot on 2015-04-06 15:18:26:

Concur

[skliper]

Trac comment by glimes on 2015-04-07 12:48:24:

Tested changeset [changeset:d8f0b8de] as part of the ic-2015-03-10 merge.

[skliper]

Trac comment by glimes on 2015-04-13 15:23:05:

Part of integration candidate 2015-03-10,
committed to cFS CFE Development branch on 2015-04-10
as part of merge [changeset:7d6f6d0].

[skliper]

Trac comment by glimes on 2016-02-25 10:17:32:

these will be fixed in CFE 6.5

[skliper]

Trac comment by jhageman on 2019-03-05 14:57:55:

Milestone renamed