Replaced xMalloc+xSnprintf with xAsprintf in PCP platform code
#1587
Conversation
|
Please remove |
kingmidas-hack
force-pushed
the
replace-xmalloc-with-xasprintf
branch
from
2dcc0d5 to
6b80b3c
Compare
|
@Explorer09 Oof my apologies. This is done! |
BenBE
added
enhancement
| @@ -71,13 +71,11 @@ static void PCPDynamicScreens_appendDynamicColumns(PCPDynamicScreens* screens, P | |||
|
|
|||
| static PCPDynamicColumn* PCPDynamicScreen_lookupMetric(PCPDynamicScreen* screen, const char* name) { | |||
| PCPDynamicColumn* column = NULL; | |||
| size_t bytes = strlen(name) + strlen(screen->super.name) + 1; /* colon */ | |||
| if (bytes >= sizeof(column->super.name)) | |||
| if ((strlen(name) + strlen(screen->super.name) + 1) >= sizeof(column->super.name)) /* colon */ | |||
There was a problem hiding this comment.
xAsprintf already returns the actual string length. Thus you could use the return value below for this check (although this means we will allocate memory here in case of overlong column names). But given this would be on the error path this should happen rarely enough to not matter.
There was a problem hiding this comment.
Hey there! I appreciate your feedback! I've just committed a change for the indentation. As for using xAsprintf's return value, I see how this helps! To my understanding and in terms of scale, it's better to allocate memory first, then check the size and free() (if necessary) - than to check the size first, then allocate? Which I guess would make sense since it handles any buffer overflow.
I have not yet tested compiling a PCP version, though brew does provide PCP tooling. I can try testing but am curious if opening another branch is necessary.
There was a problem hiding this comment.
@BenBE This looks bad. While PCPDynamicScreen_lookupMetric() could return NULL, there are no NULL checks on the only caller of the function, PCPDynamicScreen_parseColumn(). That is, you get undefined behavior with an overlong string.
There was a problem hiding this comment.
@Explorer09 This would be a bug with the current code base too. A fix would be independent of this change.
Hi! I submitted a PR in hopes this would suffice. After refactoring
xMalloc+xSnprintftoxAsprintfwithin~/pcp/PCPDynamicColumn_addMetric.c~/pcp/PCPDynamicMeter_lookupMetric.c~/pcp/PCPDynamicScreen_lookupMetric.cI was able to compile and test the changes (
make clean && make) with no hassle within macOS. I'm open to any constructive criticism for writing C and overall best practices for maintaining large codebases. Thanks all!