Guenael / rtlsdr-wsprd

WSPR daemon for RTL receivers
GNU General Public License v3.0
112 stars 32 forks source link

Possible resource leaks #42

Closed dforsi closed 2 years ago

dforsi commented 2 years ago

Running cppcheck *c in the rtlsdr-wsprd directory shows the following warnings and errors:

Checking fano.c ...
Checking fano.c: MJ...
Checking fano.c: NASA_STANDARD...
1/7 files checked 7% done
Checking nhash.c ...
Checking nhash.c: VALGRIND...
2/7 files checked 23% done
Checking rtlsdr_wsprd.c ...
rtlsdr_wsprd.c:413:9: error: Resource leak: fd [resourceLeak]
        return 1;
        ^
rtlsdr_wsprd.c:419:9: error: Resource leak: fd [resourceLeak]
        return 1;
        ^
3/7 files checked 49% done
Checking tab.c ...
4/7 files checked 50% done
Checking wsprd.c ...
wsprd.c:490:19: warning: The file '"hashtable.txt"' is opened for read and write access at the same time on different streams [incompatibleFileOpen]
            fhash=fopen("hashtable.txt","w+");
                  ^
5/7 files checked 80% done
Checking wsprd_utils.c ...
6/7 files checked 91% done
Checking wsprsim_utils.c ...
wsprsim_utils.c:123:9: error: Memory leak: call6 [memleak]
        call6=strtok(NULL," ");
        ^
7/7 files checked 100% done
Guenael commented 2 years ago

Good point, I setup CodeQL on GitHub, but running this tool is good too! I should include it in the pipeline.

But these errors are related to a some decoding options (ex. usehashtable), and I got some issues in the past, on Raspberry Pi devices, but not on regular x86 devices. RaspberryPi FS is based on low performance SD-Card, and I'm not sure what is going on. It could be related to this finding ('"hashtable.txt"' is opened for read and write access at the same time) but I'm not sure. Anyway I will fix it.

Thanks for this report! 73

Guenael commented 2 years ago

@dforsi I created this MR : https://github.com/Guenael/rtlsdr-wsprd/pull/45 but some additional tests are required. I will also do some code reformat with clang-format, cannot read this code ^^

Guenael commented 2 years ago

I guess I missing something on the call6=strtok(NULL," "); ... I could created some impact/issue

dforsi commented 2 years ago

I guess I missing something on the call6=strtok(NULL," "); ... I could created some impact/issue

I think that the problem is that memory is allocated here:

wsprd/wsprsim_utils.c:91: call6 = malloc(sizeof(char) * 6);

and that pointer is overwritten here:

wsprd/wsprsim_utils.c:122: call6 = strtok(pfx, " "); // FIXME-CHECK

so you can't free the memory allocated.

This would be easiest fix, but I only compile-tested it an cppcheck'ed:

diff --git a/wsprd/wsprsim_utils.c b/wsprd/wsprsim_utils.c index 41d2c40..d99c9d5 100644 --- a/wsprd/wsprsim_utils.c +++ b/wsprd/wsprsim_utils.c @@ -88,7 +88,9 @@ long unsigned int pack_call(char callsign) { void pack_prefix(char callsign, int32_t n, int32_t m, int32_t nadd) { int i; char call6;

@@ -147,6 +149,7 @@ void pack_prefix(char callsign, int32_t n, int32_t m, int32_t nadd) { *nadd = 1; } }

void interleave(unsigned char *sym) {

I didn't open a PR because I didn't understand why in pack_call() a "call6" is an array allocated on the stack and here in pack_prefix() it is allocated with malloc().

-- Daniele Forsi

Guenael commented 2 years ago

should be fixed now, but I will rework a part of the backend with a more recent version of wsprd.