Skip to content

Commit fba6817

Browse files
lplewalplewa
committed
fix logger when enabled thru ctl
Logger configuration failed when set via the CTL env variable because the API used const char ** while the env var provided const char *. This mismatch affected the size field (sizeof(char *)==8 vs. string length). The API now accepts char * instead of char **.
1 parent 7cc203e commit fba6817

File tree

6 files changed

+115
-49
lines changed

6 files changed

+115
-49
lines changed

‎src/ctl/ctl.c

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -198,11 +198,13 @@ static void *ctl_parse_args(const struct ctl_argument *arg_proto, char *arg) {
198198
* structure as required by the node callback
199199
*/
200200
static void *ctl_query_get_real_args(const umf_ctl_node_t *n, void *write_arg,
201-
umf_ctl_query_source_t source) {
201+
umf_ctl_query_source_t source,
202+
size_t *size) {
202203
void *real_arg = NULL;
203204
switch (source) {
204205
case CTL_QUERY_CONFIG_INPUT:
205206
real_arg = ctl_parse_args(n->arg, write_arg);
207+
*size = n->arg->dest_size;
206208
break;
207209
case CTL_QUERY_PROGRAMMATIC:
208210
real_arg = write_arg;
@@ -251,7 +253,7 @@ static umf_result_t ctl_exec_query_write(void *ctx, const umf_ctl_node_t *n,
251253
return UMF_RESULT_ERROR_INVALID_ARGUMENT;
252254
}
253255

254-
void *real_arg = ctl_query_get_real_args(n, arg, source);
256+
void *real_arg = ctl_query_get_real_args(n, arg, source, &size);
255257
if (real_arg == NULL) {
256258
return UMF_RESULT_ERROR_INVALID_ARGUMENT;
257259
}

‎src/utils/utils_log.c

Lines changed: 34 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77
*
88
*/
99

10+
#include "base_alloc_global.h"
11+
#include "umf/base.h"
1012
#ifdef _WIN32
1113
#include <windows.h>
1214
#else
@@ -66,11 +68,11 @@ typedef struct {
6668
utils_log_level_t level;
6769
utils_log_level_t flushLevel;
6870
FILE *output;
69-
const char *file_name;
71+
char file_name[MAX_FILE_PATH];
7072
} utils_log_config_t;
7173

7274
utils_log_config_t loggerConfig = {false, false, LOG_ERROR,
73-
LOG_ERROR, NULL, NULL};
75+
LOG_ERROR, NULL, ""};
7476

7577
static const char *level_to_str(utils_log_level_t l) {
7678
switch (l) {
@@ -257,10 +259,10 @@ void utils_log_init(void) {
257259
const char *arg;
258260
if (utils_parse_var(envVar, "output:stdout", NULL)) {
259261
loggerConfig.output = stdout;
260-
loggerConfig.file_name = "stdout";
262+
strncpy(loggerConfig.file_name, "stdout", MAX_FILE_PATH);
261263
} else if (utils_parse_var(envVar, "output:stderr", NULL)) {
262264
loggerConfig.output = stderr;
263-
loggerConfig.file_name = "stderr";
265+
strncpy(loggerConfig.file_name, "stderr", MAX_FILE_PATH);
264266
} else if (utils_parse_var(envVar, "output:file", &arg)) {
265267
loggerConfig.output = NULL;
266268
const char *argEnd = strstr(arg, ";");
@@ -289,7 +291,9 @@ void utils_log_init(void) {
289291
loggerConfig.output = NULL;
290292
return;
291293
}
292-
loggerConfig.file_name = file;
294+
strncpy(loggerConfig.file_name, file, MAX_FILE_PATH - 1);
295+
loggerConfig.file_name[MAX_FILE_PATH - 1] =
296+
'\0'; // ensure null-termination
293297
} else {
294298
loggerConfig.output = stderr;
295299
LOG_ERR("Logging output not set - logging disabled (UMF_LOG = \"%s\")",
@@ -506,17 +510,29 @@ static umf_result_t CTL_READ_HANDLER(output)(void *ctx,
506510
/* suppress unused-parameter errors */
507511
(void)source, (void)indexes, (void)ctx;
508512

509-
const char **arg_out = (const char **)arg;
510-
if (arg_out == NULL || size < sizeof(const char *)) {
513+
char *arg_out = (char *)arg;
514+
if (arg_out == NULL) {
511515
return UMF_RESULT_ERROR_INVALID_ARGUMENT;
512516
}
513517

514518
if (loggerConfig.output == NULL) {
515-
*arg_out = "disabled";
519+
const char disabled[] = "disabled";
520+
if (size < sizeof(disabled)) {
521+
LOG_ERR("Invalid output argument size: %zu, expected at least %zu",
522+
size, sizeof(disabled));
523+
return UMF_RESULT_ERROR_INVALID_ARGUMENT;
524+
}
525+
526+
strncpy(arg_out, disabled, size);
516527
return UMF_RESULT_SUCCESS;
517528
}
529+
if (size < strlen(loggerConfig.file_name)) {
530+
LOG_ERR("Invalid output argument size: %zu, expected at least %zu",
531+
size, strlen(loggerConfig.file_name));
532+
return UMF_RESULT_ERROR_INVALID_ARGUMENT;
533+
}
518534

519-
*arg_out = loggerConfig.file_name;
535+
strncpy(arg_out, loggerConfig.file_name, size);
520536
return UMF_RESULT_SUCCESS;
521537
}
522538

@@ -525,16 +541,13 @@ static umf_result_t CTL_WRITE_HANDLER(output)(void *ctx,
525541
void *arg, size_t size,
526542
umf_ctl_index_utlist_t *indexes) {
527543
/* suppress unused-parameter errors */
528-
(void)source, (void)indexes, (void)ctx;
544+
(void)source, (void)indexes, (void)ctx, (void)size;
529545

530-
const char *arg_in = *(const char **)arg;
531-
if (size < sizeof(const char *)) {
532-
return UMF_RESULT_ERROR_INVALID_ARGUMENT;
533-
}
546+
const char *arg_in = (const char *)arg;
534547

535548
FILE *oldHandle = loggerConfig.output;
536549
const char *oldName =
537-
loggerConfig.file_name ? loggerConfig.file_name : "disabled";
550+
*loggerConfig.file_name == '\0' ? loggerConfig.file_name : "disabled";
538551

539552
if (arg_in == NULL) {
540553
if (loggerConfig.output) {
@@ -543,7 +556,7 @@ static umf_result_t CTL_WRITE_HANDLER(output)(void *ctx,
543556
fclose(oldHandle);
544557
}
545558
loggerConfig.output = NULL;
546-
loggerConfig.file_name = NULL;
559+
loggerConfig.file_name[0] = '\0';
547560
}
548561
return UMF_RESULT_SUCCESS;
549562
}
@@ -552,16 +565,18 @@ static umf_result_t CTL_WRITE_HANDLER(output)(void *ctx,
552565

553566
if (strcmp(arg_in, "stdout") == 0) {
554567
newHandle = stdout;
555-
loggerConfig.file_name = "stdout";
568+
strncpy(loggerConfig.file_name, "stdout", MAX_FILE_PATH);
556569
} else if (strcmp(arg_in, "stderr") == 0) {
557570
newHandle = stderr;
558-
loggerConfig.file_name = "stderr";
571+
strncpy(loggerConfig.file_name, "stderr", MAX_FILE_PATH);
559572
} else {
560573
newHandle = fopen(arg_in, "a");
561574
if (!newHandle) {
562575
return UMF_RESULT_ERROR_INVALID_ARGUMENT;
563576
}
564-
loggerConfig.file_name = arg_in;
577+
strncpy(loggerConfig.file_name, arg_in, MAX_FILE_PATH - 1);
578+
loggerConfig.file_name[MAX_FILE_PATH - 1] =
579+
'\0'; // ensure null-termination
565580
}
566581

567582
loggerConfig.output = newHandle;

‎test/ctl/ctl_api.cpp

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -414,21 +414,23 @@ TEST_F(test, ctl_logger_basic_rw) {
414414
UMF_RESULT_SUCCESS);
415415
EXPECT_EQ(flush_get, 2);
416416

417-
const char *out_name = "stdout";
418-
ASSERT_EQ(umfCtlSet("umf.logger.output", &out_name, sizeof(out_name)),
419-
UMF_RESULT_SUCCESS);
420-
const char *out_get = NULL;
421-
ASSERT_EQ(umfCtlGet("umf.logger.output", &out_get, sizeof(out_get)),
417+
const char out_name[] = "stdout";
418+
ASSERT_EQ(
419+
umfCtlSet("umf.logger.output", (void *)out_name, sizeof(out_name)),
420+
UMF_RESULT_SUCCESS);
421+
const char out_get[256] = "";
422+
ASSERT_EQ(umfCtlGet("umf.logger.output", (void *)out_get, sizeof(out_get)),
422423
UMF_RESULT_SUCCESS);
423424
EXPECT_STREQ(out_get, "stdout");
424425
}
425426

426427
TEST_F(test, ctl_logger_output_file) {
427-
const char *file_name = "ctl_log.txt";
428-
ASSERT_EQ(umfCtlSet("umf.logger.output", &file_name, sizeof(file_name)),
429-
UMF_RESULT_SUCCESS);
430-
const char *out_get = NULL;
431-
ASSERT_EQ(umfCtlGet("umf.logger.output", &out_get, sizeof(out_get)),
428+
const char file_name[] = "ctl_log.txt";
429+
ASSERT_EQ(
430+
umfCtlSet("umf.logger.output", (void *)file_name, sizeof(file_name)),
431+
UMF_RESULT_SUCCESS);
432+
const char out_get[256] = "";
433+
ASSERT_EQ(umfCtlGet("umf.logger.output", (void *)out_get, sizeof(out_get)),
432434
UMF_RESULT_SUCCESS);
433435
EXPECT_STREQ(out_get, file_name);
434436
}

‎test/ctl/ctl_env_app.cpp

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,32 @@ static int test_env_defaults(int argc, char **argv) {
4141
return 0;
4242
}
4343

44+
static int test_logger(int argc, char **argv) {
45+
char buf[256] = {0};
46+
int level = 0;
47+
48+
if (argc != 2) {
49+
std::cerr << "expected two arguments" << std::endl;
50+
std::cerr << "Usage: env_defaults log_output log_level" << std::endl;
51+
return 1;
52+
}
53+
umfCtlGet("umf.logger.output", buf, sizeof(buf));
54+
if (strcmp(buf, argv[0]) != 0) {
55+
std::cerr << "Expected log_output to be '" << argv[0] << "', but got '"
56+
<< buf << "'" << std::endl;
57+
return 1;
58+
}
59+
60+
umfCtlGet("umf.logger.level", &level, sizeof(level));
61+
if (level != atoi(argv[1])) {
62+
std::cerr << "Expected log_level to be '" << argv[1] << "', but got '"
63+
<< level << "'" << std::endl;
64+
return 1;
65+
}
66+
67+
return 0;
68+
}
69+
4470
int main(int argc, char **argv) {
4571
if (argc < 2) {
4672
std::cerr << "Usage: " << argv[0] << " <test_name> args..."
@@ -53,5 +79,9 @@ int main(int argc, char **argv) {
5379
if (strcmp(test_name, "env_defaults") == 0) {
5480
return test_env_defaults(argc, argv);
5581
}
82+
83+
if (strcmp(test_name, "logger") == 0) {
84+
return test_logger(argc, argv);
85+
}
5686
return 1;
5787
}

‎test/ctl/ctl_env_driver.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,3 +121,8 @@ TEST_F(test, ctl_env_plus_file) {
121121
"opt_two_value2", "umf.pool.default.test_pool.opt_three",
122122
"second"});
123123
}
124+
125+
TEST_F(test, ctl_env_logger) {
126+
run_case({{"UMF_CONF", "umf.logger.output=stdout;umf.logger.level=0"}},
127+
{"logger", "stdout", "0"});
128+
}

‎test/utils/utils_log.cpp

Lines changed: 30 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,8 @@ void helper_checkConfig(utils_log_config_t *expected, utils_log_config_t *is) {
144144

145145
TEST_F(test, parseEnv_errors) {
146146
expected_message = "";
147-
loggerConfig = {false, false, LOG_ERROR, LOG_ERROR, NULL, ""};
147+
loggerConfig =
148+
utils_log_config_t{false, false, LOG_ERROR, LOG_ERROR, NULL, ""};
148149

149150
expect_fput_count = 0;
150151
expected_stream = stderr;
@@ -217,8 +218,8 @@ TEST_F(test, parseEnv) {
217218
flushLevel.first + ";" +
218219
output.first + ";" +
219220
timestamp.first + ";" + pid.first;
220-
b = loggerConfig = {false, false, LOG_ERROR,
221-
LOG_ERROR, NULL, ""};
221+
b = loggerConfig = utils_log_config_t{
222+
false, false, LOG_ERROR, LOG_ERROR, NULL, ""};
222223
expect_fput_count = 0;
223224
expect_fopen_count = 0;
224225
expected_stream = stderr;
@@ -290,8 +291,8 @@ TEST_F(test, log_levels) {
290291
expected_stream = stderr;
291292
for (int i = LOG_DEBUG; i <= LOG_ERROR; i++) {
292293
for (int j = LOG_DEBUG; j <= LOG_ERROR; j++) {
293-
loggerConfig = {false, false, (utils_log_level_t)i,
294-
LOG_DEBUG, stderr, ""};
294+
loggerConfig = utils_log_config_t{
295+
false, false, (utils_log_level_t)i, LOG_DEBUG, stderr, ""};
295296
if (i > j) {
296297
expect_fput_count = 0;
297298
expect_fflush_count = 0;
@@ -314,7 +315,8 @@ TEST_F(test, log_outputs) {
314315
expect_fflush_count = 1;
315316
expected_message = "[DEBUG UMF] " + MOCK_FN_NAME + ": example log\n";
316317
for (auto o : outs) {
317-
loggerConfig = {false, false, LOG_DEBUG, LOG_DEBUG, o, ""};
318+
loggerConfig =
319+
utils_log_config_t{false, false, LOG_DEBUG, LOG_DEBUG, o, ""};
318320
expected_stream = o;
319321
helper_test_log(LOG_DEBUG, MOCK_FN_NAME.c_str(), "%s", "example log");
320322
}
@@ -325,8 +327,8 @@ TEST_F(test, flush_levels) {
325327
expect_fput_count = 1;
326328
for (int i = LOG_DEBUG; i <= LOG_ERROR; i++) {
327329
for (int j = LOG_DEBUG; j <= LOG_ERROR; j++) {
328-
loggerConfig = {false, false, LOG_DEBUG, (utils_log_level_t)i,
329-
stderr, ""};
330+
loggerConfig = utils_log_config_t{
331+
false, false, LOG_DEBUG, (utils_log_level_t)i, stderr, ""};
330332
if (i > j) {
331333
expect_fflush_count = 0;
332334
} else {
@@ -343,7 +345,8 @@ TEST_F(test, flush_levels) {
343345
TEST_F(test, long_log) {
344346
expect_fput_count = 1;
345347
expect_fflush_count = 1;
346-
loggerConfig = {false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
348+
loggerConfig =
349+
utils_log_config_t{false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
347350
expected_message = "[DEBUG UMF] " + MOCK_FN_NAME + ": " +
348351
std::string(8189 - MOCK_FN_NAME.size(), 'x') + "\n";
349352
helper_test_log(LOG_DEBUG, MOCK_FN_NAME.c_str(), "%s",
@@ -358,7 +361,8 @@ TEST_F(test, long_log) {
358361
TEST_F(test, timestamp_log) {
359362
expect_fput_count = 1;
360363
expect_fflush_count = 1;
361-
loggerConfig = {true, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
364+
loggerConfig =
365+
utils_log_config_t{true, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
362366
// TODO: for now we do not check output message,
363367
// as it requires more sophisticated message validation (a.k.a regrex)
364368
expected_message = "";
@@ -368,15 +372,17 @@ TEST_F(test, timestamp_log) {
368372
TEST_F(test, pid_log) {
369373
expect_fput_count = 1;
370374
expect_fflush_count = 1;
371-
loggerConfig = {false, true, LOG_DEBUG, LOG_DEBUG, stderr, ""};
375+
loggerConfig =
376+
utils_log_config_t{false, true, LOG_DEBUG, LOG_DEBUG, stderr, ""};
372377
// TODO: for now we do not check output message,
373378
// as it requires more sophisticated message validation (a.k.a regrex)
374379
expected_message = "";
375380
helper_test_log(LOG_DEBUG, MOCK_FN_NAME.c_str(), "%s", "example log");
376381
}
377382

378383
TEST_F(test, log_fatal) {
379-
loggerConfig = {false, false, LOG_DEBUG, LOG_DEBUG, NULL, ""};
384+
loggerConfig =
385+
utils_log_config_t{false, false, LOG_DEBUG, LOG_DEBUG, NULL, ""};
380386
expected_stream = stderr;
381387
expect_fput_count = 1;
382388
expect_fflush_count = 1;
@@ -390,7 +396,8 @@ TEST_F(test, log_macros) {
390396
expected_stream = stderr;
391397
expect_fput_count = 1;
392398
expect_fflush_count = 1;
393-
loggerConfig = {false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
399+
loggerConfig =
400+
utils_log_config_t{false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
394401

395402
expected_message = "[DEBUG UMF] TestBody: example log\n";
396403
fput_count = 0;
@@ -437,7 +444,8 @@ template <typename... Args> void helper_test_plog(Args... args) {
437444
}
438445

439446
TEST_F(test, plog_basic) {
440-
loggerConfig = {false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
447+
loggerConfig =
448+
utils_log_config_t{false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
441449
expected_stream = stderr;
442450
errno = 1;
443451
strerr = "test error";
@@ -453,7 +461,8 @@ TEST_F(test, plog_basic) {
453461
}
454462

455463
TEST_F(test, plog_invalid) {
456-
loggerConfig = {false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
464+
loggerConfig =
465+
utils_log_config_t{false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
457466
expected_stream = stderr;
458467
errno = INVALID_ERRNO;
459468
strerr = "test error";
@@ -469,7 +478,8 @@ TEST_F(test, plog_invalid) {
469478
}
470479

471480
TEST_F(test, plog_long_message) {
472-
loggerConfig = {false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
481+
loggerConfig =
482+
utils_log_config_t{false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
473483
expected_stream = stderr;
474484
expect_fput_count = 1;
475485
expect_fflush_count = 1;
@@ -490,7 +500,8 @@ TEST_F(test, plog_long_message) {
490500
}
491501

492502
TEST_F(test, plog_long_error) {
493-
loggerConfig = {false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
503+
loggerConfig =
504+
utils_log_config_t{false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
494505
expected_stream = stderr;
495506
expect_fput_count = 1;
496507
expect_fflush_count = 1;
@@ -516,7 +527,8 @@ TEST_F(test, log_pmacros) {
516527
expected_stream = stderr;
517528
expect_fput_count = 1;
518529
expect_fflush_count = 1;
519-
loggerConfig = {false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
530+
loggerConfig =
531+
utils_log_config_t{false, false, LOG_DEBUG, LOG_DEBUG, stderr, ""};
520532
errno = 1;
521533
strerr = "test error";
522534

0 commit comments

Comments
 (0)