stripydog / kplex

kplex marine data multiplexer
Other
82 stars 38 forks source link

[BUG] A use after free bug in options.c #60

Open ShangzhiXu opened 1 year ago

ShangzhiXu commented 1 year ago

File: options.c Bug Function: sfilter_t getfilter(char fstring) Version: Git-master/kplex-1.4

Description: I'll use the Git-master version as an example. In line 235-241, *filter, *tfilter and **fptrare declared; In line 245-248, *tfilter is inited. Then in line 300, if ((*fstring == FILTERDELIM) || (*fstring == '\0')) is true, then the following code will be executed:

(*fptr)=tfilter;
 fptr=&tfilter->next;
ok=1;

When try to free them in line 319-324 with :

    if (tfilter)
        free(tfilter);
    for(;filter;filter=tfilter) {
        tfilter=filter->next;
        free(filter);

It is likely to cause a memory misuse bug.

If we let fptr=&filter at line 241, and then call (*fptr)=tfilter; in line 301, thefilterwill be signed with tfilter, so after free(tfilter); the program will get crash atfree(filter);

stripydog commented 1 year ago

Thank you for taking the time to report this and my sincere apologies for taking so long to reply. The fact that I had to stare so long at this to remind myself how it works reminds me that this code is too complicated, although in fairness it's a long time since I wrote this!

This is a bug but it looks like (correct me if I'm wrong) it will only get triggered if the malloc at line 310 fails. The block of code you highlight starting at line 300 sets ok = 1. Then if it's the end of the filter string (*fstring == \0) it breaks out of the loop with ok=1 and if the malloc at line 310 is successful, the block at 319 is never reached. If the end of the filter rule is the delimiter character ":", we go round the loop again and the first action is to malloc a new tfilter. if that fails and we break out of the loop, filter is null so doesn't get freed at 319. if it succeeds it's not connected to anything until line 301.

The block at 319 should be: if (filter && !ok)

I'll update this. Thanks for spotting