Skip to content

Improve use of sprintf and snprintf #46

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
luke-jr merged 10 commits into from
Mar 7, 2025
Merged

Improve use of sprintf and snprintf #46

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b628b89
Improve use of sprintf and snprintf
jesterhodl Dec 10, 2024
96d3b1f
handle truncation in vsnprintf and reduce size of char time_buffer[] …
jesterhodl Dec 14, 2024
90e6b02
- in order to stop populating userpass over and over again, populate …
jesterhodl Dec 15, 2024
42666a2
Merge branch 'OCEAN-xyz:master' into better-sprintf-snprintf
jesterhodl Dec 15, 2024
599c38f
Merge branch 'OCEAN-xyz:master' into better-sprintf-snprintf
jesterhodl Jan 17, 2025
c320b45
use datum_config.bitcoind_rpcuserpass instead of a global
jesterhodl Jan 25, 2025
f192e90
Clearer sizing of max_string_len for extra_block_submissions
jesterhodl Jan 25, 2025
e199667
Resize coinbase tags to better values
jesterhodl Jan 25, 2025
8edf26e
Reduce and rename DATUM_CONFIG_MAX_STRING_ARRAY_LEN
jesterhodl Jan 25, 2025
2b7442b
Put rpcuser/pass from global back to datum config
jesterhodl Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions src/datum_blocktemplates.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,6 @@ T_DATUM_TEMPLATE_DATA *datum_gbt_parser(json_t *gbt) {

void *datum_gateway_fallback_notifier(void *args) {
CURL *tcurl = NULL;
char userpass[512];
char req[512];
char p1[72];
p1[0] = 0;
Expand All @@ -345,10 +344,8 @@ void *datum_gateway_fallback_notifier(void *args) {
}
DLOG_DEBUG("Fallback notifier thread ready.");

sprintf(userpass, "%s:%s", datum_config.bitcoind_rpcuser, datum_config.bitcoind_rpcpassword);

while(1) {
sprintf(req, "{\"jsonrpc\":\"1.0\",\"id\":\"%"PRIu64"\",\"method\":\"getbestblockhash\",\"params\":[]}", current_time_millis());
snprintf(req, sizeof(req), "{\"jsonrpc\":\"1.0\",\"id\":\"%"PRIu64"\",\"method\":\"getbestblockhash\",\"params\":[]}", current_time_millis());
gbbh = json_rpc_call(tcurl, datum_config.bitcoind_rpcurl, userpass, req);
if (gbbh) {
res_val = json_object_get(gbbh, "result");
Expand Down Expand Up @@ -383,7 +380,6 @@ void *datum_gateway_template_thread(void *args) {
uint64_t i = 0;
char gbt_req[1024];
int j;
char userpass[512];
T_DATUM_TEMPLATE_DATA *t;
bool was_notified = false;
int wnc = 0;
Expand Down Expand Up @@ -411,13 +407,11 @@ void *datum_gateway_template_thread(void *args) {
char p1[72];
p1[0] = 0;

sprintf(userpass, "%s:%s", datum_config.bitcoind_rpcuser, datum_config.bitcoind_rpcpassword);

while(1) {
i++;

// fetch latest template
sprintf(gbt_req, "{\"method\":\"getblocktemplate\",\"params\":[{\"rules\":[\"segwit\"]}],\"id\":%"PRIu64"}",(uint64_t)((uint64_t)time(NULL)<<(uint64_t)8)|(uint64_t)(i&255));
snprintf(gbt_req, sizeof(gbt_req), "{\"method\":\"getblocktemplate\",\"params\":[{\"rules\":[\"segwit\"]}],\"id\":%"PRIu64"}",(uint64_t)((uint64_t)time(NULL)<<(uint64_t)8)|(uint64_t)(i&255));
gbt = json_rpc_call(tcurl, datum_config.bitcoind_rpcurl, userpass, gbt_req);

if (!gbt) {
Expand Down
40 changes: 26 additions & 14 deletions src/datum_conf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,17 +47,19 @@
#include "datum_sockets.h"

global_config_t datum_config;
// since we null-terminate the user and password, we'll have enough for the semicolon and NUL
char userpass[sizeof(datum_config.bitcoind_rpcuser) + sizeof(datum_config.bitcoind_rpcpassword)];

const char *datum_conf_var_type_text[] = { "N/A", "boolean", "integer", "string", "string_array" };

const T_DATUM_CONFIG_ITEM datum_config_options[] = {
// Bitcoind configs
{ .var_type = DATUM_CONF_STRING, .category = "bitcoind", .name = "rpcuser", .description = "RPC username for communication with local bitcoind.",
.required = true, .ptr = datum_config.bitcoind_rpcuser, .max_string_len = 128 },
.required = true, .ptr = datum_config.bitcoind_rpcuser, .max_string_len = sizeof(datum_config.bitcoind_rpcuser) },
{ .var_type = DATUM_CONF_STRING, .category = "bitcoind", .name = "rpcpassword", .description = "RPC password for communication with local bitcoind.",
.required = true, .ptr = datum_config.bitcoind_rpcpassword, .max_string_len = 128 },
.required = true, .ptr = datum_config.bitcoind_rpcpassword, .max_string_len = sizeof(datum_config.bitcoind_rpcpassword) },
{ .var_type = DATUM_CONF_STRING, .category = "bitcoind", .name = "rpcurl", .description = "RPC URL for communication with local bitcoind. (GBT Template Source)",
.required = true, .ptr = datum_config.bitcoind_rpcurl, .max_string_len = 128 },
.required = true, .ptr = datum_config.bitcoind_rpcurl, .max_string_len = sizeof(datum_config.bitcoind_rpcurl) },
{ .var_type = DATUM_CONF_INT, .category = "bitcoind", .name = "work_update_seconds", .description = "How many seconds between normal work updates? (5-120, 40 suggested)",
.required = false, .ptr = &datum_config.bitcoind_work_update_seconds, .default_int = 40 },
{ .var_type = DATUM_CONF_BOOL, .category = "bitcoind", .name = "notify_fallback", .description = "Fall back to less efficient methods for new block notifications. Can disable if you use blocknotify.",
Expand Down Expand Up @@ -93,23 +95,23 @@ const T_DATUM_CONFIG_ITEM datum_config_options[] = {

// mining settings
{ .var_type = DATUM_CONF_STRING, .category = "mining", .name = "pool_address", .description = "Bitcoin address used for mining rewards.",
.required = true, .ptr = datum_config.mining_pool_address, .max_string_len = 128 },
.required = true, .ptr = datum_config.mining_pool_address, .max_string_len = sizeof(datum_config.mining_pool_address) },
{ .var_type = DATUM_CONF_STRING, .category = "mining", .name = "coinbase_tag_primary", .description = "Text to have in the primary coinbase tag when not using pool (overridden by DATUM Pool)",
.required = false, .ptr = datum_config.mining_coinbase_tag_primary, .default_string[0] = "DATUM Gateway", .max_string_len = 64 },
.required = false, .ptr = datum_config.mining_coinbase_tag_primary, .default_string[0] = "DATUM Gateway", .max_string_len = sizeof(datum_config.mining_coinbase_tag_primary) },
{ .var_type = DATUM_CONF_STRING, .category = "mining", .name = "coinbase_tag_secondary", .description = "Text to have in the secondary coinbase tag (Short name/identifier)",
.required = false, .ptr = datum_config.mining_coinbase_tag_secondary, .default_string[0] = "DATUM User", .max_string_len = 64 },
.required = false, .ptr = datum_config.mining_coinbase_tag_secondary, .default_string[0] = "DATUM User", .max_string_len = sizeof(datum_config.mining_coinbase_tag_secondary) },
{ .var_type = DATUM_CONF_INT, .category = "mining", .name = "coinbase_unique_id", .description = "A unique ID between 1 and 65535. This is appended to the coinbase. Make unique per instance of datum with the same coinbase tags.",
.required = false, .ptr = &datum_config.coinbase_unique_id, .default_int = 4242 },
{ .var_type = DATUM_CONF_STRING, .category = "mining", .name = "save_submitblocks_dir", .description = "Directory to save all submitted blocks to as submitblock JSON files",
.required = false, .ptr = datum_config.mining_save_submitblocks_dir, .default_string[0] = "", .max_string_len = 256 },
.required = false, .ptr = datum_config.mining_save_submitblocks_dir, .default_string[0] = "", .max_string_len = sizeof(datum_config.mining_save_submitblocks_dir) },

// API/dashboard
{ .var_type = DATUM_CONF_INT, .category = "api", .name = "listen_port", .description = "Port to listen for API/dashboard requests (0=disabled)",
.required = false, .ptr = &datum_config.api_listen_port, .default_int = 0 },

// extra block submissions list
{ .var_type = DATUM_CONF_STRING_ARRAY, .category = "extra_block_submissions", .name = "urls", .description = "Array of bitcoind RPC URLs to submit our blocks to directly. Include auth info: http://user:pass@IP",
.required = false, .ptr = datum_config.extra_block_submissions_urls[0], .max_string_len = 512 },
.required = false, .ptr = datum_config.extra_block_submissions_urls[0], .max_string_len = sizeof(datum_config.extra_block_submissions_urls[0]) },

// logger
{ .var_type = DATUM_CONF_BOOL, .category = "logger", .name = "log_to_console", .description = "Enable logging of messages to the console",
Expand All @@ -119,7 +121,7 @@ const T_DATUM_CONFIG_ITEM datum_config_options[] = {
{ .var_type = DATUM_CONF_BOOL, .category = "logger", .name = "log_to_file", .description = "Enable logging of messages to a file",
.required = false, .ptr = &datum_config.clog_to_file, .default_bool = false },
{ .var_type = DATUM_CONF_STRING, .category = "logger", .name = "log_file", .description = "Path to file to write log messages, when enabled",
.required = false, .ptr = datum_config.clog_file, .default_string[0] = "", .max_string_len = 1023 },
.required = false, .ptr = datum_config.clog_file, .default_string[0] = "", .max_string_len = sizeof(datum_config.clog_file) },
{ .var_type = DATUM_CONF_BOOL, .category = "logger", .name = "log_rotate_daily", .description = "Rotate the message log file at midnight",
.required = false, .ptr = &datum_config.clog_rotate_daily, .default_bool = true },
{ .var_type = DATUM_CONF_BOOL, .category = "logger", .name = "log_calling_function", .description = "Log the name of the calling function when logging",
Expand All @@ -131,11 +133,11 @@ const T_DATUM_CONFIG_ITEM datum_config_options[] = {

// datum options
{ .var_type = DATUM_CONF_STRING, .category = "datum", .name = "pool_host", .description = "Remote DATUM server host/ip to use for decentralized pooled mining (set to \"\" to disable pooled mining)",
.required = false, .ptr = datum_config.datum_pool_host, .default_string[0] = "datum-beta1.mine.ocean.xyz", .max_string_len = 1023 },
.required = false, .ptr = datum_config.datum_pool_host, .default_string[0] = "datum-beta1.mine.ocean.xyz", .max_string_len = sizeof(datum_config.datum_pool_host) },
{ .var_type = DATUM_CONF_INT, .category = "datum", .name = "pool_port", .description = "Remote DATUM server port",
.required = false, .ptr = &datum_config.datum_pool_port, .default_int = 28915 },
{ .var_type = DATUM_CONF_STRING, .category = "datum", .name = "pool_pubkey", .description = "Public key of the DATUM server for initiating encrypted connection. Get from secure location, or set to empty to auto-fetch.",
.required = false, .ptr = datum_config.datum_pool_pubkey, .default_string[0] = "f21f2f0ef0aa1970468f22bad9bb7f4535146f8e4a8f646bebc93da3d89b1406f40d032f09a417d94dc068055df654937922d2c89522e3e8f6f0e649de473003", .max_string_len = 1023 },
.required = false, .ptr = datum_config.datum_pool_pubkey, .default_string[0] = "f21f2f0ef0aa1970468f22bad9bb7f4535146f8e4a8f646bebc93da3d89b1406f40d032f09a417d94dc068055df654937922d2c89522e3e8f6f0e649de473003", .max_string_len = sizeof(datum_config.datum_pool_pubkey) },
{ .var_type = DATUM_CONF_BOOL, .category = "datum", .name = "pool_pass_workers", .description = "Pass stratum miner usernames as sub-worker names to the pool (pool_username.miner's username)",
.required = false, .ptr = &datum_config.datum_pool_pass_workers, .default_bool = true },
{ .var_type = DATUM_CONF_BOOL, .category = "datum", .name = "pool_pass_full_users", .description = "Pass stratum miner usernames as raw usernames to the pool (use if putting multiple payout addresses on miners behind this gateway)",
Expand Down Expand Up @@ -221,8 +223,11 @@ int datum_config_parse_value(const T_DATUM_CONFIG_ITEM *c, json_t *item) {
return 1;
}
if (!json_is_string(item)) return -1;
strncpy((char *)c->ptr, json_string_value(item), c->max_string_len-1);
((char *)c->ptr)[c->max_string_len-1] = 0;
// check for overflow
int written = snprintf((char *)c->ptr, c->max_string_len, "%s", json_string_value(item));
if (written >= c->max_string_len) {
return -2;
}
return 1;
}

Expand Down Expand Up @@ -286,15 +291,22 @@ int datum_read_config(const char *conffile) {

// item might be valid
j = datum_config_parse_value(&datum_config_options[i], item);
if (j != 1) {
if (j == -1) {
DLOG_ERROR("Could not parse configuration option %s.%s. Type should be %s", datum_config_options[i].category, datum_config_options[i].name, (datum_config_options[i].var_type<DATUM_CONF_TYPES)?datum_conf_var_type_text[datum_config_options[i].var_type]:"UNKNOWN");
return -1;
} else if (j == -2) {
DLOG_ERROR("Configuration option %s.%s exceeds maximum length of %d", datum_config_options[i].category, datum_config_options[i].name, datum_config_options[i].max_string_len);
return -1;
}
}

if (config) {
json_decref(config);
}

// populate userpass for further reuse
snprintf(userpass, sizeof(userpass), "%s:%s", datum_config.bitcoind_rpcuser, datum_config.bitcoind_rpcpassword);

// pass configuration options to the logger
datum_logger_config(datum_config.clog_to_file, datum_config.clog_to_console, datum_config.clog_level_console, datum_config.clog_level_file, datum_config.clog_calling_function, datum_config.clog_to_stderr, datum_config.clog_rotate_daily, datum_config.clog_file);

Expand Down
5 changes: 3 additions & 2 deletions src/datum_conf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ typedef struct {

// Globally accessable config options
typedef struct {
char bitcoind_rpcuser[256];
char bitcoind_rpcpassword[256];
char bitcoind_rpcuser[128];
char bitcoind_rpcpassword[128];
Comment on lines +71 to +72
Copy link
Copy Markdown
Contributor

@luke-jr luke-jr Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just replace these with bitcoind_rpcuserpass rather than the userpass global?

Copy link
Copy Markdown
Contributor Author

@jesterhodl jesterhodl Jan 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, removed the global. In order to easily parse user and pass from the config I made rpcuser and rpcpassword char arrays in datum_conf.c. Is what's in the last commit ok? c320b45

Copy link
Copy Markdown
Contributor

@luke-jr luke-jr Jan 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That moves the individual user/pass to globals :(

Maybe check out what I did in #60

Copy link
Copy Markdown
Contributor Author

@jesterhodl jesterhodl Jan 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I initially wanted to just put rpcuserpass in global_config_t, and I really wanted to some how get rid of bitcoind_rpcuser and bitcoind_rpcpassword from it as you said (replace). That would require copying the user and password parts into the same array and "rejigging" it to put the colon in (%s:%s)

That would allow to get rid of rpcuser and rpcpassword, but I didn't like it.
Which do you prefer: trying to drop rpcuser and rpcpassword completely or keeping all 3 (user, pass, userpass)?

Copy link
Copy Markdown
Contributor Author

@jesterhodl jesterhodl Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, so I thought you originally wanted to completely remove rpcuser and rpcpassword in favour of a joint field, that's why my previous comment was kinda odd/stupid :-|
The #60 shows you're ok with putting userpass to the config, so I did that.
I can rebase after you get #60 in.

char bitcoind_rpcurl[256];
int bitcoind_work_update_seconds;
bool bitcoind_notify_fallback;
Expand Down Expand Up @@ -126,6 +126,7 @@ typedef struct {
} global_config_t;

extern global_config_t datum_config;
extern char userpass[sizeof(datum_config.bitcoind_rpcuser) + sizeof(datum_config.bitcoind_rpcpassword)];

int datum_read_config(const char *conffile);
void datum_gateway_help(void);
Expand Down
18 changes: 11 additions & 7 deletions src/datum_logger.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ int datum_logger_queue_msg(const char *func, int level, const char *format, ...)
va_list args;
struct timeval tv;
struct tm tm_info;
char time_buffer[40];
char time_buffer[20];

if ((level < log_level_console) && (level < log_level_file)) {
return 0;
Expand Down Expand Up @@ -202,7 +202,12 @@ int datum_logger_queue_msg(const char *func, int level, const char *format, ...)
msg->msg = &msg_buffer[buffer_id][msg_buf_idx[buffer_id]];
va_start(args, format);
i = vsnprintf(msg->msg, 1023, format, args);
msg->msg[i] = 0;

// clamp i to actual written value in order not to waste buffer space
if (i >= 1023) {
i = 1022;
}

va_end(args);

if (((msg_buf_idx[buffer_id]+i+2) > msg_buf_maxsz) || (dlog_queue_next[buffer_id] >= dlog_queue_max_entries)) {
Expand Down Expand Up @@ -251,7 +256,7 @@ void * datum_logger_thread(void *ptr) {
struct tm tm_info_storage;
struct tm *tm_info;
DLOG_MSG *msg;
char time_buffer[40];
char time_buffer[20];
char log_line[1200];
FILE *log_handle = NULL;
time_t next_log_rotate = get_midnight_timestamp();
Expand Down Expand Up @@ -348,9 +353,9 @@ void * datum_logger_thread(void *ptr) {
tm_info = localtime_r(&seconds, &tm_info_storage);
strftime(time_buffer, sizeof(time_buffer), "%Y-%m-%d %H:%M:%S", tm_info);
if (log_calling_function) {
j = snprintf(log_line, 1199, "%s.%03d [%44s] %s: %s\n", time_buffer, millis, msg->calling_function, level_text[msg->level], msg->msg);
j = snprintf(log_line, sizeof(log_line), "%s.%03d [%44s] %s: %s\n", time_buffer, millis, msg->calling_function, level_text[msg->level], msg->msg);
} else {
j = snprintf(log_line, 1199, "%s.%03d %s: %s\n", time_buffer, millis, level_text[msg->level], msg->msg);
j = snprintf(log_line, sizeof(log_line), "%s.%03d %s: %s\n", time_buffer, millis, level_text[msg->level], msg->msg);
}
log_line[1199] = 0;
Copy link
Copy Markdown
Contributor

@luke-jr luke-jr Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should drop this?

Copy link
Copy Markdown
Contributor Author

@jesterhodl jesterhodl Jan 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we set NUL at 1199 anyway? We can. I just wanted to reduce relying on hardcoded lengths and make the code use defined char arrays sizes and recommended ways to call the "snprintf family".

char log_line[1200];

// ...

if (log_calling_function) {
                                        j = snprintf(log_line, sizeof(log_line), "%s.%03d [%44s] %s: %s\n", time_buffer, millis, msg->calling_fu>
                                } else {
                                        j = snprintf(log_line, sizeof(log_line), "%s.%03d %s: %s\n", time_buffer, millis, level_text[msg->level]>
                                }

What do you think?


Expand Down Expand Up @@ -390,8 +395,7 @@ void * datum_logger_thread(void *ptr) {

tm_info = localtime_r(&log_file_opened, &tm_info_storage);
strftime(time_buffer, sizeof(time_buffer), "%Y-%m-%d", tm_info);
snprintf(log_line, 1199, "%s.%s", log_file, time_buffer);
log_line[1199] = 0;
snprintf(log_line, sizeof(log_line), "%s.%s", log_file, time_buffer);

fclose(log_handle);

Expand Down
Loading