Code guidelines suggestions

[adwait1-g]

Description

A few good practices that will improve the code.

1. Replace malloc with calloc everywhere.
2. Use r_free instead of free - this will zeroize the pointer which is being freed.
3. Initialize local variables to a definite value (generally zeroize everything)
4. Codebase has strcpy and such unsafe functions - which should be replaced by their safer variants (strncpy).

I believe the above 4 can be done without much effort/decision making.

Below are a few things we'll have to discuss and decide on.

1. Would it be a good idea to use the safeC library (https://rurban.github.io/safeclib/doc/safec-3.0/index.html) instead of plain C functions? - it'll enforce stronger checks(NULL checks, boundary/length checks etc.,) - hardens code against corruption and crashes.
2. A few tools are leaking memory (which can be easily caught on valgrind) - can we aim to fix all the memory leaks? Do you think it is necessary at this stage or can it be ignored?

Regards, Adwaith

[richardpl]

1. can impact performance
2. not always possible
3. this can hide bugs
4. strcpy is safe if used in safe way

[trufae]

Good suggestions!

some of those changes were planned already, so let me elaborate:

1 for malloc/free i plan to change them all in 5.8 with macros named r_malloc() and r_free() this way we can use a different allocator at compile time and also add early return versions to check for null

2 we have R_FREE which nulls the pointer. Theres no need to nullify the contents before freeing. It may seem safer but if sthg is accessed theres a bug somewhere else

3 coverity and last gcc and valgrind are able to catch most. Not to say all uninitialized variables. It may affect performance as richard said

4 strcpy is a very bad function and should be avoided to avoid bad uses. Most of the time theres a better way to rewrite the code than using strcpy. Im in favour to add a lint for this

[swoops]

IMO strncpy is more scary than strcpy because it's danger is subtler. I know strcpy is unsafe just from it's prototype. However, strncpy can cause leaks and even overflows if you don't ensure the string is null terminated after the copy. Consider:

#include <stdio.h>
#include <string.h>
int main(int argc, char *argv[]) {
	char buf1[4];
	char buf2[4];
	if (argc == 3) {
		strncpy (buf1, argv[1], 4);
		strncpy (buf2, argv[2], 4);
		printf("buf1: %s\n", buf1);
		printf("buf2: %s\n", buf2);
	}
	return 0;
}

The code above seems safe but:

./a.out AAAA BBBB
buf1: AAAA
buf2: BBBBAAAA

Now the buf2 string is longer then expected which could lead to an overflow depending how it's used. Additionally, it's only by chance that buf1 ends with a NULL byte, otherwise there would be a stack leak, potentially exposing the stack canary. I believe snprintf has this behavior as well.

Changing the strncpy size from 4 to 3 does not help, the same behavior will be seen as long as buf2[3] is not a NULL.

So I would rather have a r_strncpy available that always NULL terminates.

[trufae]

Agree strncpy is also dangerous and should be forbidden in r2land

[condret]

I think that strcpy and strncpy have their valid niches in r2 codebase (copy const strings to destinations of guaranteed length, doing copy operations within apis like r_strbuf).

I don't see the argument for replacing all calls to malloc (R_NEW) with calloc (R_NEW0) in our existing codebase. But i don't have a strong opinion on that.

Local vars should only be declared on or nearby first write access.

We're aware where the memleaks come from, and it's a rather structural problem instead of being caused by the usage of certain "unsafe" functions.

@swoops snprintf is better