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 1 commit
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
12 changes: 6 additions & 6 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,7 @@ T_DATUM_TEMPLATE_DATA *datum_gbt_parser(json_t *gbt) {

void *datum_gateway_fallback_notifier(void *args) {
CURL *tcurl = NULL;
char userpass[512];
char userpass[1024];
Copy link
Copy Markdown
Contributor

@luke-jr luke-jr Dec 14, 2024

Choose a reason for hiding this comment

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

Each of user and pass accept up to 1023 bytes, so maybe for consistency this should be (1023 * 2) + 2?

Copy link
Copy Markdown
Contributor Author

@jesterhodl jesterhodl Dec 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There' further inconsistency.
datum_conf.h says:

typedef struct {
        char bitcoind_rpcuser[256];
        char bitcoind_rpcpassword[256];

datum_conf.c has max_string_length set to 128 and it does truncate to 128

so maximum userpass would be even less: 127*2+2, but I suggest to focus this one on the userpass and snprintf bits and leave aligning that with datum_conf.c and datum_conf.h in a seprate PR.

Copy link
Copy Markdown
Contributor

@luke-jr luke-jr Dec 14, 2024

Choose a reason for hiding this comment

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

Indeed, I was mixing it up with pool_host/pubkey.

Just be careful with the one case it's reused for a file path.

Copy link
Copy Markdown
Contributor Author

@jesterhodl jesterhodl Dec 15, 2024

Choose a reason for hiding this comment

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

Aside from the path reuse case. How about we populate the userpass once at startup, and access as a config variable, instead of reconstructing it all the time in 5 files?

Copy link
Copy Markdown
Contributor Author

@jesterhodl jesterhodl Dec 15, 2024

Choose a reason for hiding this comment

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

Next commit shows what I meant. Do the userpass thing just one time, keep it global, similarly to how is set once datum_protocol_global_timeout_ms. This eliminates all those multiple snprintf()s
I've also thrown in some other goodies:

  • in order to stop populating userpass over and over again, populate once and reuse
  • in order to eliminate discrepancies between char array sizes, use sizeof() in max_string_length of datum_config_options
  • introduce a warning in case a config option is truncated
  • where userpass was reused for a filepath, create a separate variable
  • reduce user and pass to 128 bytes
  • exit during config upon parsing error, makes more sense to me

Copy link
Copy Markdown
Contributor Author

@jesterhodl jesterhodl Dec 21, 2024

Choose a reason for hiding this comment

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

Is this fine?

char req[512];
char p1[72];
p1[0] = 0;
Expand All @@ -345,10 +345,10 @@ 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);
snprintf(userpass, sizeof(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 +383,7 @@ void *datum_gateway_template_thread(void *args) {
uint64_t i = 0;
char gbt_req[1024];
int j;
char userpass[512];
char userpass[1024];
T_DATUM_TEMPLATE_DATA *t;
bool was_notified = false;
int wnc = 0;
Expand Down Expand Up @@ -411,13 +411,13 @@ 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);
snprintf(userpass, sizeof(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
9 changes: 4 additions & 5 deletions src/datum_logger.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ int datum_logger_queue_msg(const char *func, int level, const char *format, ...)
msg->calling_function[47] = 0;
msg->msg = &msg_buffer[buffer_id][msg_buf_idx[buffer_id]];
va_start(args, format);
i = vsnprintf(msg->msg, 1023, format, args);
i = vsnprintf(msg->msg, sizeof(msg->msg), format, args);
Copy link
Copy Markdown
Contributor

@luke-jr luke-jr Dec 14, 2024

Choose a reason for hiding this comment

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

msg->msg is a char*, so this will only be the size of a pointer and truncate all log messages...

Copy link
Copy Markdown
Contributor Author

@jesterhodl jesterhodl Dec 14, 2024

Choose a reason for hiding this comment

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

Ouch, it shows I didn't check the underlying type because I thought this is the place with a char[]. Sorry!

We can revert to 1023, especially because log_line that gets written to the file is 1200, so if we subtract about 80 bytes for the timestamp, function name and log level, we could fit about 1100 bytes, so that's fine.

But I found something else here: the return value of vsnprintf is not checked and has an effect.

In case of messages larger than 1023 it would waste a bit of the circular buffer. This is because when vsnprintf truncates it returns the untruncated length of the data. So, if it is >= 1023, then msg_buf_idx is moved by more than it really needs to, causing waste of the buffer. eg. 4000 instead of 1023:
msg_buf_idx[buffer_id] += i+2; // moved by 4000 but we truncated to 1023

If I am reading this right then I would suggest a quick check of the return value.

        i = vsnprintf(msg->msg, 1023, format, args);

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

Thoughts?

Copy link
Copy Markdown
Contributor

@luke-jr luke-jr Dec 14, 2024

Choose a reason for hiding this comment

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

Sounds fine

msg->msg[i] = 0;
va_end(args);

Expand Down Expand Up @@ -348,9 +348,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 +390,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
45 changes: 22 additions & 23 deletions src/datum_stratum.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -690,7 +690,7 @@ void datum_stratum_v1_socket_thread_loop(T_DATUM_THREAD_DATA *my) {
void send_error_to_client(T_DATUM_CLIENT_DATA *c, uint64_t id, char *e) {
// "e" must be valid JSON string
char s[1024];
snprintf(s, 1023, "{\"error\":%s,\"id\":%"PRIu64",\"result\":null}\n", e, id);
snprintf(s, sizeof(s), "{\"error\":%s,\"id\":%"PRIu64",\"result\":null}\n", e, id);
datum_socket_send_string_to_client(c, s);
}

Expand Down Expand Up @@ -1283,7 +1283,7 @@ int client_mining_submit(T_DATUM_CLIENT_DATA *c, uint64_t id, json_t *params_obj
}

char s[256];
snprintf(s, 255, "{\"error\":null,\"id\":%"PRIu64",\"result\":true}\n", id);
snprintf(s, sizeof(s), "{\"error\":null,\"id\":%"PRIu64",\"result\":true}\n", id);
datum_socket_send_string_to_client(c, s);

// update connection totals
Expand Down Expand Up @@ -1370,15 +1370,15 @@ int client_mining_configure(T_DATUM_CLIENT_DATA *c, uint64_t id, json_t *params_
}
}

i = snprintf(sa, 1023, "{\"error\":null,\"id\":%"PRIu64",\"result\":{", id);
i = snprintf(sa, sizeof(sa), "{\"error\":null,\"id\":%"PRIu64",\"result\":{", id);
if (new_vroll) {
i+= snprintf(&sa[i], 1023-i, "\"version-rolling\":true,\"version-rolling.mask\":\"%08x\"", m->extension_version_rolling_mask);
i+= snprintf(&sa[i], sizeof(sa)-i, "\"version-rolling\":true,\"version-rolling.mask\":\"%08x\"", m->extension_version_rolling_mask);
}
if (new_mdiff) {
// we don't currently support miner specified minimum difficulty.
i+= snprintf(&sa[i], 1023-i, ",\"minimum-difficulty\":false");
i+= snprintf(&sa[i], sizeof(sa)-i, ",\"minimum-difficulty\":false");
}
i+= snprintf(&sa[i], 1023-i, "}}\n");
i+= snprintf(&sa[i], sizeof(sa)-i, "}}\n");

datum_socket_send_string_to_client(c, sa);

Expand Down Expand Up @@ -1406,10 +1406,10 @@ int client_mining_authorize(T_DATUM_CLIENT_DATA *c, uint64_t id, json_t *params_
}
}

strncpy(m->last_auth_username, username_s, 191);
m->last_auth_username[191] = 0;
strncpy(m->last_auth_username, username_s, sizeof(m->last_auth_username) - 1);
m->last_auth_username[sizeof(m->last_auth_username)-1] = 0;

snprintf(s, 255, "{\"error\":null,\"id\":%"PRIu64",\"result\":true}\n", id);
snprintf(s, sizeof(s), "{\"error\":null,\"id\":%"PRIu64",\"result\":true}\n", id);
datum_socket_send_string_to_client(c, s);

m->authorized = true;
Expand Down Expand Up @@ -1523,12 +1523,12 @@ int send_mining_notify(T_DATUM_CLIENT_DATA *c, bool clean, bool quickdiff, bool
// new block work always is just a blank coinbase, for now

if (quickdiff) {
snprintf(s, 511, "\"Q%s%2.2x\",\"%s\",\"", j->job_id, cbselect, j->prevhash);
snprintf(s, sizeof(s), "\"Q%s%2.2x\",\"%s\",\"", j->job_id, cbselect, j->prevhash);
} else {
if (!new_block) {
snprintf(s, 511, "\"%s%2.2x\",\"%s\",\"", j->job_id, cbselect, j->prevhash);
snprintf(s, sizeof(s), "\"%s%2.2x\",\"%s\",\"", j->job_id, cbselect, j->prevhash);
} else {
snprintf(s, 511, "\"N%s%2.2x\",\"%s\",\"", j->job_id, 255, j->prevhash); // empty coinbase for new block
snprintf(s, sizeof(s), "\"N%s%2.2x\",\"%s\",\"", j->job_id, 255, j->prevhash); // empty coinbase for new block
cb = &j->subsidy_only_coinbase;
}
}
Expand Down Expand Up @@ -1571,7 +1571,7 @@ int send_mining_notify(T_DATUM_CLIENT_DATA *c, bool clean, bool quickdiff, bool
// send empty merkle leafs
datum_socket_send_string_to_client(c, "[]");
}
snprintf(s, 511, ",\"%s\",\"%s\",\"%s\",", j->version, j->nbits, j->ntime);
snprintf(s, sizeof(s), ",\"%s\",\"%s\",\"%s\",", j->version, j->nbits, j->ntime);
datum_socket_send_string_to_client(c, s);

// bunch of reasons we may need to discard old work
Expand All @@ -1594,7 +1594,7 @@ int send_mining_set_difficulty(T_DATUM_CLIENT_DATA *c) {
m->current_diff = datum_config.stratum_v1_vardiff_min;
}

snprintf(s, 255, "{\"id\":null,\"method\":\"mining.set_difficulty\",\"params\":[%"PRIu64"]}\n", (uint64_t)m->current_diff);
snprintf(s, sizeof(s), "{\"id\":null,\"method\":\"mining.set_difficulty\",\"params\":[%"PRIu64"]}\n", (uint64_t)m->current_diff);
datum_socket_send_string_to_client(c, s);

m->last_sent_diff = m->current_diff;
Expand Down Expand Up @@ -1712,7 +1712,7 @@ int client_mining_subscribe(T_DATUM_CLIENT_DATA *c, uint64_t id, json_t *params_
m->sid_inv = ((sid>>24)&0xff) | (((sid>>16)&0xff)<<8) | (((sid>>8)&0xff)<<16) | ((sid&0xff)<<24);

// tell them about all of this
snprintf(s, 1023, "{\"error\":null,\"id\":%"PRIu64",\"result\":[[[\"mining.notify\",\"%8.8x1\"],[\"mining.set_difficulty\",\"%8.8x2\"]],\"%8.8x\",8]}\n", id, sid, sid, sid);
snprintf(s, sizeof(s), "{\"error\":null,\"id\":%"PRIu64",\"result\":[[[\"mining.notify\",\"%8.8x1\"],[\"mining.set_difficulty\",\"%8.8x2\"]],\"%8.8x\",8]}\n", id, sid, sid, sid);
datum_socket_send_string_to_client(c, s);

// send them their current difficulty before sending a job
Expand Down Expand Up @@ -1967,14 +1967,14 @@ void update_stratum_job(T_DATUM_TEMPLATE_DATA *block_template, bool new_block, i
}
s->prevhash[64] = 0;

snprintf(s->version, 9, "%8.8x", block_template->version);
snprintf(s->version, sizeof(s->version), "%8.8x", block_template->version);
s->version_uint = block_template->version;
strncpy(s->nbits, block_template->bits, 9);
strncpy(s->nbits, block_template->bits, sizeof(s->nbits) - 1);

// TODO: Should we use local time, and just verify is valid for the block?
// Perhaps as an option.
// The template's time is 100% safe, so we'll use that for now.
snprintf(s->ntime, 9, "%8.8llx", (unsigned long long)block_template->curtime);
snprintf(s->ntime, sizeof(s->ntime), "%8.8llx", (unsigned long long)block_template->curtime);

// Set the coinbase value of this job based on the template
s->coinbase_value = block_template->coinbasevalue;
Expand Down Expand Up @@ -2054,7 +2054,7 @@ void update_stratum_job(T_DATUM_TEMPLATE_DATA *block_template, bool new_block, i
}

s->global_index = global_latest_stratum_job_index;
snprintf(s->job_id, 23, "%8.8x%2.2x%4.4x", (uint32_t)time(NULL), (uint8_t)stratum_job_next, (uint16_t)s->global_index ^ STRATUM_JOB_INDEX_XOR);
snprintf(s->job_id, sizeof(s->job_id), "%8.8x%2.2x%4.4x", (uint32_t)time(NULL), (uint8_t)stratum_job_next, (uint16_t)s->global_index ^ STRATUM_JOB_INDEX_XOR);

global_cur_stratum_jobs[global_latest_stratum_job_index] = s;
pthread_rwlock_unlock(&stratum_global_job_ptr_lock);
Expand All @@ -2071,7 +2071,7 @@ int assembleBlockAndSubmit(uint8_t *block_header, uint8_t *coinbase_txn, size_t
size_t i;
json_t *r;
CURL *tcurl;
char userpass[512];
char userpass[1024];
Copy link
Copy Markdown
Contributor

@luke-jr luke-jr Dec 14, 2024

Choose a reason for hiding this comment

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

(same as above)

Copy link
Copy Markdown
Contributor Author

@jesterhodl jesterhodl Dec 14, 2024

Choose a reason for hiding this comment

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

as above

int ret = 0;
bool free_submitblock_req = false;
char *s = NULL;
Expand Down Expand Up @@ -2140,8 +2140,7 @@ int assembleBlockAndSubmit(uint8_t *block_header, uint8_t *coinbase_txn, size_t
if (datum_config.mining_save_submitblocks_dir[0] != 0) {
// save the block submission to a file named by the block's hash
FILE *f;
snprintf(userpass, 511, "%s/datum_submitblock_%s.json", datum_config.mining_save_submitblocks_dir, block_hash_hex);
userpass[511] = 0;
snprintf(userpass, sizeof(userpass), "%s/datum_submitblock_%s.json", datum_config.mining_save_submitblocks_dir, block_hash_hex);
f = fopen(userpass, "w");
if (!f) {
DLOG_ERROR("Could not open %s for writing submitblock record to disk: %s!", userpass, strerror(errno));
Expand All @@ -2162,7 +2161,7 @@ int assembleBlockAndSubmit(uint8_t *block_header, uint8_t *coinbase_txn, size_t
return 0;
}

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

// make the call!
r = json_rpc_call(tcurl, datum_config.bitcoind_rpcurl, userpass, submitblock_req);
Expand Down
9 changes: 3 additions & 6 deletions src/datum_submitblock.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,8 @@ void preciousblock(CURL *curl, char *blockhash) {
char userpass[1024];
Copy link
Copy Markdown
Contributor

@luke-jr luke-jr Dec 14, 2024

Choose a reason for hiding this comment

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

(same as above, and also for datum_submitblock_doit)

Copy link
Copy Markdown
Contributor Author

@jesterhodl jesterhodl Dec 14, 2024

Choose a reason for hiding this comment

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

as above


// TODO: Move these types of things to the conf file
snprintf(userpass, 1023, "%s:%s", datum_config.bitcoind_rpcuser, datum_config.bitcoind_rpcpassword);
userpass[1023] = 0;

sprintf(rpc_data,"{\"method\":\"preciousblock\",\"params\":[\"%s\"],\"id\":1}",blockhash);
snprintf(userpass, sizeof(userpass), "%s:%s", datum_config.bitcoind_rpcuser, datum_config.bitcoind_rpcpassword);
snprintf(rpc_data, sizeof(rpc_data), "{\"method\":\"preciousblock\",\"params\":[\"%s\"],\"id\":1}", blockhash);
json = json_rpc_call(curl, datum_config.bitcoind_rpcurl, userpass, rpc_data);
if (!json) return;

Expand All @@ -72,8 +70,7 @@ void datum_submitblock_doit(CURL *tcurl, char *url, const char *submitblock_req,
char *s = NULL;
// TODO: Move these types of things to the conf file
if (!url) {
snprintf(userpass, 1023, "%s:%s", datum_config.bitcoind_rpcuser, datum_config.bitcoind_rpcpassword);
userpass[1023] = 0;
snprintf(userpass, sizeof(userpass), "%s:%s", datum_config.bitcoind_rpcuser, datum_config.bitcoind_rpcpassword);

r = json_rpc_call(tcurl, datum_config.bitcoind_rpcurl, userpass, submitblock_req);
} else {
Expand Down