Add library attribute for no-failure allocators

[b4n]

For example GLib and GTK abort on most allocation failures, making the nullPointerOutOfMemory check produce lots of false positives.

To fix this, introduce a new allocator attribute "no-fail" to notify that this allocator cannot fail and that this check is thus irrelevant for it.

[b4n]

There probably are other options for tagging the library functions, I can update if wanted.

[firewave]

This also needs to be added in the Library Editor in the GUI (see gui/cppchecklibrarydata.cpp) as well as the GUI tests in gui/test/cppchecklibrarydata (those should fail without the editor integration).

[firewave]

Could you also provide a link to documentation that supports this behavior?

[b4n]

Could you also provide a link to documentation that supports this behavior?

e.g. https://docs.gtk.org/glib/func.malloc.html:

If the allocation fails (because the system is out of memory), the program is terminated.

The idea behind this is that allocation failure for most trivial allocations are hard to handle well, and likely will end up terminating the program anyway. More classic allocation is available with e.g. g_try_malloc() when ready to deal with potential allocation issues (usually targeted at larger or optional allocations).

[firewave]

Thanks.

Allocates n_bytes bytes of memory. If n_bytes is 0 it returns NULL.

That indicates that some code still needs the check afterwards. So it might be worth reviewing the calls anyways.

The idea behind this is that allocation failure for most trivial allocations are hard to handle well, and likely will end up terminating the program anyway.

Yes, OOM handling/recovery is very hard to impossible.
Everything major I worked on would just trigger an immediate shutdown in that case. On some systems that would never happen though because the OOM Killer was over-eagerly terminating the process in memory pressure situations.

[b4n]

Allocates n_bytes bytes of memory. If n_bytes is 0 it returns NULL.

That indicates that some code still needs the check afterwards. So it might be worth reviewing the calls anyways.

Yes, in some cases NULL is still a possible result, while not being an allocation failure. However, many, like g_string_new() will never return NULL because they either succeed at allocating their internal structure, or they will terminate.

The cases of e.g. g_malloc(0) or g_strdup(NULL) will indeed return NULL and might need checking, but IMO those are both rare, or other checks should be able to find issues: e.g. ptr = g_malloc(n) where n=0 should catch all access of ptr above or equal to 0 -- and thus see an out of bound access on anything.

As things are right now, most GLib-using code will just produce gazillion false-positives (see e.g. geany/geany-plugins#1417): even the few checks in gtk.cfg required overrides to pass in the first place. So even if we say that it should catch the g_malloc(0) case, ignoring it is a worthy trade-off IMO as as-is I would simply disable/ignore the check everywhere (and I don't know of a way to disable that one alone, but I might just lack configuration knowledge, I'm not (yet?) a guru user 🙂 )
However, if you can think of a more general solution that can express those specific cases, why not.

[firewave]

However, if you can think of a more general solution that can express those specific cases, why not.

I think this is as generic as it gets for APIs. But it is up to @danmar to decide.

Regarding cases in terms of the standard see the previous thread in #7068.

[firewave]

Please also add an entry with the new attribute to the GUI test data.

[b4n]

@firewave you mean setting it to true in one of the tests, or is there another place I missed?

[firewave]

@firewave you mean setting it to true in one of the tests, or is there another place I missed?

Yes. I think that would be an additional entry in gui/test/cppchecklibrarydata/files/memory_resource_valid.cfg.

[danmar]

@b4n thanks for your contribution.

[danmar]

The name "no-fail" is not ideal. But I don't have better suggestion directly. Something like returns-nonnull maybe.. We can switch relatively easy in the near future if we want.

[firewave]

We should still file a ticket about the false positive so this is documented.

I also think it might be missing in the library data editor (but that should have failed a test!?).

[b4n]

@danmar it isn't, but returns-nonnull is not great either because as mentioned in #7390 (comment) some specific calls like g_malloc(0) can return NULL, it's just never the result of an allocation failure but the API contract.
But I agree that the name isn't perfect, if anybody can come up with a better one soon enough it's indeed welcome.

[b4n]

@firewave didn't I fix that in the latest revision? There's no UI for it sure, but it's not the only option that is not available there I believe, and it should be loaded/saved properly now.

[firewave]

didn't I fix that in the latest revision? There's no UI for it sure, but it's not the only option that is not available there I believe, and it should be loaded/saved properly now.

Ah, it is being loaded but not displayed. The latter can obviously not being tested easily. Guess we need to review the UI and file a ticket about it.

[danmar]

We should still file a ticket about the false positive so this is documented.

Oops!! I will create a ticket! I really shouldn't have merged this without a ticket :-(

Guess we need to review the UI and file a ticket about it.

I am not against it however..
My feeling is that the UI is not very nice and user friendly :-(
I have no data at all but I don't think that people use it currently.
Will anybody complain if we remove it rather than maintaining it..

[firewave]

Oops!! I will create a ticket! I really shouldn't have merged this without a ticket :-(

It happens.

My feeling is that the UI is not very nice and user friendly :-(

It beats editing a raw XML file.

I have no data at all but I don't think that people use it currently.
Will anybody complain if we remove it rather than maintaining it..

I assume a few people actually use - otherwise we wouldn't have gotten several PRs extending it in the past.

I think we can keep it because it is very static in nature so there is not much in terms of support.

[firewave]

The memory functions have no UI mask at all so there was nothing to add here. Having support for that is covered by https://trac.cppcheck.net/ticket/10688.