Static analyzers see opt_args_data as leaked (but the leak is O(1) therefore not a real problem)

[jmarrero]

Possible RESOURCE_LEAK on scan:

56. bubblewrap-0.9.0/bubblewrap.c:1703:11: alloc_fn: Storage is returned from allocation function "load_file_data".
57. bubblewrap-0.9.0/bubblewrap.c:1703:11: var_assign: Assigning: "opt_args_data" = storage returned from "load_file_data(the_fd, &data_len)".
59. bubblewrap-0.9.0/bubblewrap.c:1708:11: var_assign: Assigning: "data_end" = "opt_args_data".
60. bubblewrap-0.9.0/bubblewrap.c:1711:11: var_assign: Assigning: "p" = "opt_args_data".
64. bubblewrap-0.9.0/bubblewrap.c:1718:15: identity_transfer: Passing "p" as argument 1 to function "memchr", which returns an offset off that argument.
65. bubblewrap-0.9.0/bubblewrap.c:1718:15: noescape: Resource "p" is not freed or pointed-to in "memchr".
66. bubblewrap-0.9.0/bubblewrap.c:1718:15: var_assign: Assigning: "p" = storage returned from "memchr(p, 0, data_end - p)".
71. bubblewrap-0.9.0/bubblewrap.c:1726:11: var_assign: Assigning: "p" = "opt_args_data".
74. bubblewrap-0.9.0/bubblewrap.c:1742:9: leaked_storage: Variable "data_end" going out of scope leaks the storage it points to.
75. bubblewrap-0.9.0/bubblewrap.c:1742:9: leaked_storage: Variable "p" going out of scope leaks the storage it points to.
91. bubblewrap-0.9.0/bubblewrap.c:1703:11: overwrite_var: Overwriting "opt_args_data" in "opt_args_data = load_file_data(the_fd, &data_len)" leaks the storage that "opt_args_data" points to.
#  1701|              * keep allocated until exit time, since its argv entries get used
#  1702|              * by the other cases in parse_args_recurse() when we recurse. */
#  1703|->           opt_args_data = load_file_data (the_fd, &data_len);
#  1704|             if (opt_args_data == NULL)
#  1705|               die_with_error ("Can't read --args data");

I see the comment on: https://github.com/containers/bubblewrap/blob/main/bubblewrap.c#L1700
that this is done on purpose. So we don't want to free opt_args_data before opt_args_data = load_file_data (the_fd, &data_len); But do we want to free it at the end of the loop? Or is there a way to refactor this that would not end up with this finding.

or... maybe this is just a false positive.

[smcv]

This is known, and #426 attempted to fix it, but as I pointed out while reviewing #426, the change proposed there isn't actually valid.

The static analyzer that you are using is sort-of-correct to say that this is a leak - but it's leaking O(1) bytes per run of bwrap, because we only parse the command-line once. So it is not actually a practical problem, and does not indicate a bug or a security vulnerability.

Back in 2021, I said:

I think opt_args_data might need to become a linked list, or some other mechanism that keeps all of the arguments blobs referenced so that static analyzers know they are OK to "leak" once per process.

If you want to contribute a merge request for that, I'd consider it. But we have to trade off "keeping static analyzers happy" against adding complexity that could introduce genuine bugs, so I'm not going to merge an implementation unless it's obvious that it's valid.