Clang static analyzer displays warning for nginx_cache_keyfinder

[hroost]

Hey, thanks a lot for a module and @iclukas for a keyfinder. I'm not familiar with Clang and static analyzers but my recent check displays warning for

Line50

ptr = end - key2len;

V769 The 'end' pointer in the 'end - key2len' expression could be nullptr. In such case, resulting value will be senseless and it should not be used.

Direct link to analyzer example. Description for this type of warnings.

So the question: is it safe to use this С module and how appropriate that warning?

[iclukas]

That’s a valid concern and might indeed lead to unexpected behaviour. As long as you don’t use the second key part, you’re totally safe.

I’m not sure how to deal with a situation like that. I think the helper should skip the file and probably return a non-zero exit code.

I’ll look into it

[diego-treitos]

I believe that there can be a problem also if there is a file without new lines (i.e. an empty file) inside the cache directory tree. In this case, ptr will be null.

I have not a lot of experience with C but maybe something like this??

   50   │ end = memchr(ptr, '\n', bytes_read - keylen);
~  51   │ if (end) {
~  52   │ │ ptr = end - key2len;
~  53   │ │ if (memcmp(ptr, key2, key2len) != 0)
+  54   │ │ │ return FTW_CONTINUE;
+  55   │ }

[iclukas]

It’s worse. If a key in a cache file was too long, \n would not be found and end be NULL. Files that are too short are handled in line 40 (now 41).

[diego-treitos]

Ahh right. I missed that check. Ok, how about this?

   50   │ end = memchr(ptr, '\n', bytes_read - keylen);
~_ 51   │ if (!end || memcmp(end - key2len, key2, key2len) != 0)
   52   │ │ return FTW_CONTINUE;

I am not even sure if the syntax is right

[iclukas]

No, you missed my PR #5

[diego-treitos]

Sorry, it seems also missed your PR. I've just merged it.

@iclukas Thanks a lot for taking your time to fix this!

@hroost Thanks for reporting it

[iclukas]

@diego-treitos Thanks for the merge.

@hroost Good find, indeed! Can you please confirm there’s no more issue with the analyzer?

[hroost]

Thanks guys for your time and quick responses! Now it doesn't rise any warnings :)