Need to get and set 64 bit numbers in getdns datastructures

[huitema]

The Microsoft compiler is somewhat stricter than gcc. The current code compiles, but produces 90 warnings.

In particular, the compiler complains about signed/unsigned comparisons, and about loss of data in assignments assignments, e.g., from size_t to uint16_t. These can be fixed with explicit casting. The compiler also protest usage of "strdup", which is deemed unsafe or obsolete.

[huitema]

Here is the actual list of warnings:

getdns_warning_list.txt

[wcawijngaards]

Hi Christian,

The gldns code is also present in unbound (in sldns/) and this is how I fixed the downcast warnings there. You can apply the patch to getdns (in the relevant location).

Best regards, Wouter

[wtoorop]

Thanks Wouter, but we don't have to apply it manually, there are synchronisation scripts and I've just used them to apply your updates :)

[wtoorop]

Hi Christian,

This warning:

val_secalgo.c(424): warning C4013: 'gldns_key_buf2rsa_raw' undefined; assuming extern returning int

might be because you miss

#define GLDNS_BUILD_CONFIG_HAVE_SSL 1

In config.h ?

[huitema]

Willem, you are correct. The warnings in val_secalgo.c disappear if config.h defines GLDNS_BUILD_CONFIG_HAVE_SSL and HAVE_OPENSSL_ERR_H.

Wouter, I can apply your patches to gldns/str2wire.c and gldns/wire2str.c, but given Willem's message I am not sure that I should. Willem, are you going to synchronize these files with the unbound version?

I will prepare a PR with just the fixes to the getdns/src/*.[ch] files, not touching gldns for now.

[huitema]

I am seeing a downcast warning for

getdns_dict_set_int( netreq_debug, "idle timeout in ms", idle_timeout)

the timeout is defined as 64 bits in the "upstream" structure, but the "dict" union sets it as 32 bits. There was a similar "clipping" issue that I fixed yesterday, but it brings the question of whether GetDNS intends to support timers larger than 4000 seconds. If we do, there should be a "getdns_dict_set_int64" variant. If not, maybe the timeout field should be set to 32 bits...

[huitema]

I just filed a PR with the proposed fixes for the GetDNS code. I did not fix the GLDNS code, and also did not fix the downcast of timeout to 32 bits, as I am not sure of the correct behavior.

Here is the new compiler output: getdns_warning_list.txt

[wtoorop]

Thanks, I synced with Wouter's fixes and did some of my own based on the additional warnings given by the -Wextra and -Wpedantic flags of gcc (as suggested by Shane). Furthermore, the CI tpkg tests will now fail when there is a warning. And yes, we need to be able to set and get 64 bit numbers in getdns datastructures. I'll change the topic of this issue to reflect this.

[huitema]

I am looking at the 32/64 bit issue. If we want to fix it, we should create two additional procedures for getdns_dict_set_int64 and getdns_dict_set_int64, and call them instead of the "int" variant when handling 64 bit integers -- I believe the only example now is the timeout delay. This will reduce the amount of churn in the code, and thus the risk of regression.

[huitema]

I looked at was is required to support 64 bit numbers in the "dict" structure. Some of that is easy: define a data type t_int64, and the procedures to get/set 64 bit int values. But then it gets more complicated. The code uses JSNM for conversion, so we have to add support for 64 bit integers in that. But then, JSON is loosely type, so the only way to differentiate 64 bit integer from 32 bit integer is the length of the numeric string. Even so, a number stored as 64 bit, printed in JSON and parsed back, will be converted to 32 bit if the value is less than 0xFFFFFFFF. Which means that "get 64 bit value" needs to succeed even if the value is stored as "t_int". Similarly, for app compat, "get 32 bit value" also need to succeed if the value is stored as t_int64 but is not greater than 0xFFFFFFFF.

All that is doable of course, but it seems that the only 64 bit structure is used for timers, which are expressed in milliseconds and never get larger than 0xFFFFFFFF. So I am proposing a simpler fix to get rid of compile time warning...

[saradickinson]

@wtoorop @huitema Can you update as to if there is anything outstanding left to fix under this issue or if it can be closed? Thanks!

[huitema]

We do not have a compile issue anymore, so in a sense the issue is mitigated. The cost of mitigation is a limit on timer values, which cannot be greater that 4 billion milliseconds, about 49 days. We can file the issue as "postponed" -- it will reappear if we do need to store int values greater than 64 bits in a dict.