fix memory misuse potential in json_stream_sampler #1360
Conversation
JSON_LIST_VALUE_setter now: checks for malloc failure in asprintf stops potential use after free on rec_type_name puts out error messages if malloc fails checks for rec_idx being set properly before use.
|
@narategithub does this actually do anything. It seems like the metric lookup would fail if the memory allocation fails in asprintf. Also if the asprintf memory allocation fails LERROR would almost certainly fail as well. @baallan, is this fix to quell a compiler/tools warning? |
| @@ -379,14 +379,28 @@ int JSON_LIST_VALUE_setter(ldms_set_t set, ldms_mval_t list_mval, | |||
| LDMS_V_CHAR_ARRAY, 255); | |||
| break; | |||
| case JSON_DICT_VALUE: | |||
| rec_idx = -1; | |||
| if (!rec_type_name) { | |||
| rc = asprintf(&rec_type_name, "%s_record", (char *)ctxt); | |||
There was a problem hiding this comment.
@tom95858 This patch covers the case that there is an error in asprintf (rc < 0) here. This case was not handled before this patch.
@tom95858 This fix is to fix actual bugs (as listed). What drew inspection of the code was a build fail under -Werr, as noted. Anecdotally, it's usually easy for asprint to succeed after a large data allocation request fails. |
|
On Tue, Feb 13, 2024 at 9:22 AM Benjamin Allan ***@***.***> wrote:
@narategithub <https://github.com/narategithub> does this actually do
anything. It seems like the metric lookup would fail if the memory
allocation fails in asprintf. Also if the asprintf memory allocation fails
LERROR would almost certainly fail as well. @baallan
<https://github.com/baallan>, is this fix to quell a compiler/tools
warning?
@tom95858 <https://github.com/tom95858> This fix is to fix actual bugs
(as listed). What drew inspection of the code was a build fail under -Werr,
as noted.
No it's not, it's to shut the compiler up...which is a valid concern.
Anecdotally, it's usually easy for asprint to succeed after a large data
allocation request fails.
This is quite simply not the case.
Separately, if the ovis_log implementation doesn't deliver at least the
first 1k of error message, we need to fix it;
We are not talking anything like that. These machines have virtual memory
backed by a disk and gigabytes of RAM. If you get an OOM, you are talking
either a ulimit issue or a truly gigantic process. 1k of messages?
it is trivial to guarantee delivery (read as 'avoid or go around fails in
asprint') by including in each log object a base buffer of 1k. This buffer
can be used until snprintf reports a truncation, or it can be used for a
fallback to snprintf when asprintf reports alloc failure (probably
preferred). Either ensures the first 1k bytes of any error get delivered,
which is of course critical to explaining memory allocation failures.
This is solving a problem that does not exist.
I simply want to confirm that we are fixing a -Wall -Werr issue. Please
confirm and then lets fix it.
… —
Reply to this email directly, view it on GitHub
<#1360 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABVTPXFRMJ3SGBECJZ72A4DYTOHODAVCNFSM6AAAAABDFLNQ3GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBRHE2DANBVHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Thomas Tucker, President, Open Grid Computing, Inc.
|
|
@tom95858 this is hpc: independent of daemon process limits, we run out of memory regularly with memory hungry applications and there is almost never backing storage. However, memory reserved when the daemon starts is not subject to the whims of later application overallocating. And, yes, as stated in the original PR description, this also addresses (validated) compiler warnings. |
JSON_LIST_VALUE_setter now:
checks for malloc failure in asprintf
stops potential use after free on rec_type_name
puts out error messages if malloc fails
checks for rec_idx being set properly before use.
stops cross-iteration control value pollution.
This also shuts up several gcc warnings on gcc 4.8.