Improve use of sprintf and snprintf by jesterhodl · Pull Request #46 · OCEAN-xyz/datum_gateway

jesterhodl · 2024-12-10T22:20:21Z

A handful of code quality improvements in how we sprintf to buffers:

Use sizeof() in snprintf to reduce the amount of hardcoded magic values that might be needed to update in the future consistently and therefore potentially lead to bugs. Also using sizeof() can catch certain classes of issues at compile time, eg. warning: ‘snprintf’ output may be truncated before the last format character [-Wformat-truncation=] or note: ‘snprintf’ output between 2 and 512 bytes into a destination of size 511
use 1024 bytes for userpass buffer consistently, this was found by using sizeof() above, actually
C99 snprintf() is OK to be called without subtracting 1 byte, we can stop bother to null terminate
prefer safer snprintf() instead sprintf() in places I found it makes sense

luke-jr · 2024-12-14T01:51:31Z

src/datum_blocktemplates.c

 void *datum_gateway_fallback_notifier(void *args) {
 	CURL *tcurl = NULL;
-	char userpass[512];
+	char userpass[1024];


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

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.

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

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

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?

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

Is this fine?

luke-jr · 2024-12-14T02:02:39Z

src/datum_logger.c

 	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);


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

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?

Sounds fine

luke-jr · 2024-12-14T02:12:09Z

src/datum_stratum.c

 	json_t *r;
 	CURL *tcurl;
-	char userpass[512];
+	char userpass[1024];


(same as above)

luke-jr · 2024-12-14T02:13:20Z

src/datum_submitblock.c

@@ -55,10 +55,8 @@ void preciousblock(CURL *curl, char *blockhash) {
 	char userpass[1024];


(same as above, and also for datum_submitblock_doit)

…to 20 20 is sufficient

…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

luke-jr · 2025-01-13T03:38:33Z

src/datum_conf.h

+	char bitcoind_rpcuser[128];
+	char bitcoind_rpcpassword[128];


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

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

That moves the individual user/pass to globals :(

Maybe check out what I did in #60

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)?

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.

src/datum_conf.c

luke-jr · 2025-01-13T03:55:18Z

src/datum_logger.c

-					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;


Should drop this?

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?

Instead of sizing the first element, size the type Co-authored-by: Luke Dashjr <luke_github1@dashjr.org>

In fact the maximums are enforced to be not more than 60 in datum_conf.c

Improve use of sprintf and snprintf

b628b89

luke-jr requested changes Dec 14, 2024

View reviewed changes

luke-jr added this to the v0.2.2 milestone Dec 14, 2024

handle truncation in vsnprintf and reduce size of char time_buffer[] …

96d3b1f

…to 20 20 is sufficient

jesterhodl force-pushed the better-sprintf-snprintf branch from 260ced7 to f35cb70 Compare December 15, 2024 18:53

jesterhodl force-pushed the better-sprintf-snprintf branch from 12e1d8f to 90e6b02 Compare December 15, 2024 19:02

Merge branch 'OCEAN-xyz:master' into better-sprintf-snprintf

42666a2

jesterhodl requested a review from luke-jr December 29, 2024 19:36

luke-jr requested changes Jan 13, 2025

View reviewed changes

luke-jr modified the milestones: v0.2.2, v0.2.3 Jan 13, 2025

jesterhodl and others added 5 commits January 17, 2025 12:01

Merge branch 'OCEAN-xyz:master' into better-sprintf-snprintf

599c38f

use datum_config.bitcoind_rpcuserpass instead of a global

c320b45

Clearer sizing of max_string_len for extra_block_submissions

f192e90

Instead of sizing the first element, size the type Co-authored-by: Luke Dashjr <luke_github1@dashjr.org>

Resize coinbase tags to better values

e199667

In fact the maximums are enforced to be not more than 60 in datum_conf.c

Reduce and rename DATUM_CONFIG_MAX_STRING_ARRAY_LEN

8edf26e

jesterhodl mentioned this pull request Feb 6, 2025

Add support for rpccookiefile alternative to rpcuser/rpcpassword #60

Merged

Put rpcuser/pass from global back to datum config

2b7442b

luke-jr merged commit 9bfb2f6 into OCEAN-xyz:master Mar 7, 2025

		@@ -55,10 +55,8 @@ void preciousblock(CURL curl, char blockhash) {
		char userpass[1024];

Conversation

jesterhodl commented Dec 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jesterhodl Dec 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jesterhodl Dec 14, 2024 •

edited

Loading