RIOT-OS / RIOT

RIOT - The friendly OS for IoT
https://riot-os.org
GNU Lesser General Public License v2.1
4.87k stars 1.98k forks source link

Tracker: Reduce scope on unintended COMMON variables #2346

Closed jnohlgard closed 1 year ago

jnohlgard commented 9 years ago

See #2344 for a description of the error.

This is a tracker list of global variables without clear scoping (either static or extern)

The following files have global variables defined which may incorrectly get treated as COMMON symbols when linking, if any other module now or in the future defines another global with the same name. This behaviour can lead to corruption of buffers, (because two drivers have accidentally been allocated the same buffer area in RAM only because they used the same name for the variable) and a range of other problems.

The solution to these problems is to add -fno-common to CFLAGS for all platforms to catch these kinds of unintended aliasing and reduce the scope to static of any global variables that are not meant to be accessed outside of the current source file.

Below is a list of global variables that are not declared static and do not have a corresponding extern declaration in any header file. The filenames and line numbers of the definitions are shown below each variable. These variables should most likely be changed to static.

It is a very long list, and some of these may not be that important once -fno-common is in place. The most important ones are the variables which are defined in more than one file.

Line numbers are based on current master (2016.03-devel-996-g7b4e776)

LudwigKnuepfer commented 9 years ago

Wow, wouldn't it have been just as much work to rename them? ;)

jnohlgard commented 9 years ago

@LudwigOrtmann nah, I cheated.

for f in $(cat globals.txt|sort|uniq); do find -name '*.h' -exec egrep "extern .* ${f}" -q {} + || (echo " - [ ] $f" && (find -name '*.c' -exec ctags -x --c-kinds=v --file-scope=no --sort=yes {} + | grep "^${f} "| awk -F ' ' '{ printf "    - [ ] %s:%d `", $4,$3; for(i=5;i<=NF;i++)printf "%s",$i (i==NF?"":OFS); printf "`\n"; }';) ) ; done > globals.md
LudwigKnuepfer commented 9 years ago

I just checked - _native_argv is declared extern in cpu/native/include/native_internal.h.

jnohlgard commented 9 years ago

There was a bug in my script. I am regenerating the list now.

jnohlgard commented 9 years ago

Updated. List shrunk from 864 to 720 items.

jnohlgard commented 9 years ago

2352 reduces the severity of this issue by introducing error detection for unintended common variables.

miri64 commented 7 years ago

Bump.

cladmi commented 7 years ago

I can try looking into this to dig a bit more in the repository. First remove easy ones, like tests, so it narrows down to the most important ones.

@gebart Can you provide the command you run to get the output please ?

What is the idea for tunslip tools, should they be fixed, or should they stay as close as possible to contiki version ?

What should be the solution for the xxx_saul_driver structures ? I do not think they should go to the default driver implementation header, so should they have each there own header, or a common big one ?. They are only used by `sys/auto_init/saul/auto_init_DRIVER/'.

The current situation is actually not good, for 'adc', the const keyword has been dropped.

This question will also be asked for all internal kernel symbol definitions.

cladmi commented 7 years ago

There may be symbols not found, like bf in tests/bloom_bytes/main.c which is defined with BITFIELD(bf, BLOOM_BITS);

jnohlgard commented 7 years ago

I will try to find what line was used to generate the globals.txt file above, the second stage of the list generation is the oneliner pasted in my comment https://github.com/RIOT-OS/RIOT/issues/2346#issuecomment-71177366

Don't bother with tunslip, it's a big mess and it is only built by itself standalone so it doesn't matter what is exported or not. The saul question about where the struct goes and why it's not made const is a separate issue and it would be more visible if you discuss it in a separate thread.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

maribu commented 1 year ago

Since compilation is done now with -fno-common (and apparently works), the issue seems to be resolved:

https://github.com/RIOT-OS/RIOT/blob/86f9d7953d2b06243ce4e3fa837c3dd21dafc9ed/makefiles/cflags.inc.mk#L60-L61