fix: malloc checks at msc_tree

[wooffie]

what

1. Check for malloc success
2. Avoid check for NULL after dereference (at memset)

why

Avoid NPD

references

Some more info: #2890

P.S. Maybe at 419 we should return something another?

[wooffie]

Also, maybe we need NULL check for node here?

ModSecurity/src/utils/msc_tree.cc

Line 685 in 1ff9f2a

if (memcmp(node->prefix->buffer, ipdata, bytes) == 0) {

[sonarqubecloud]

Quality Gate failed

Failed conditions
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

[airween]

Hi @wooffie,

thanks again for this PR.

Honestly, I would avoid malloc() calls in a C++ code. May be it's a bit bigger work, but if we want to modernize the code, we should modify the TreeNode structure: replace unsigned char * by std::vector<unsigned char>, and replace malloc() AND memset() by node->netmasks.assign(node->count, 0);.

But of course, to apply these modifications, we have to review all occurrences and places where this structure is used, and align the code.

I assume this change will disappear the SonarCloud quality issue (now you can see in check list that it's failed, because of the code complexity (with the new if() it increased that level which Sonar reports).

What do you think?

[wooffie]

Yes, no problem, I check, this is one of three files with using malloc in src. I can test changes by test, or not?

[airween]

Yes, no problem, I check, this is one of three files with using malloc in src.

Thank you!

I can test changes by test, or not?

I think you can, but may be we have to review existing tests too.

It seems like this classes and structures are used in @ipMatch and similar operators, and there is a test for this (not exactly, but I think for test is enough), and there are some other tests where the case uses @ipMatch operator.

There is a seclang-test file too, you can run that too.

Let me know if you need any help!

[wooffie]

@airween we really need to save C-API for msc_tree.h ?
Like for all structs and so on?

[airween]

@airween we really need to save C-API for msc_tree.h ? Like for all structs and so on?

Sorry, you mean you would rewrite that part and modernize the code? If yes, I think that would be awesome!

Btw I think these two files are legacy from mod_security2 code base (which is pure C source), see msc_tree.h and msc_tree.c, this is why this part is in C (too - like many others).

[wooffie]

I'm about C-compat with extern "C" linking for this header file. I think we should rewrite in without C. I will start to work around, but can't say when back with some enhancements

[airween]

I'm about C-compat with extern "C" linking for this header file.

But this header is not accessible through (library's) C-API, only library's components access them.

I think we should rewrite in without C.

Agree.

I will start to work around, but can't say when back with some enhancements

No problem, as I wrote, it's a legacy and we've been using it for about 8 years or so :)

[wooffie]

I close this one,
If I find time to rewrite it, I'll come back.