Patrik Lundin <patrik.lundin....@gmail.com> writes:
> On Sun, May 18, 2014 at 07:24:00PM +0200, Patrik Lundin wrote:
>>
>> First up is this one:
>> ===
>> pin.c: In function 'hsm_shm_open':
>> pin.c:209: warning: comparison between signed and unsigned
>> ===
>>
>> Next we have this one:
>> ===
>> hsmspeed.c:38:1: warning: "PTHREAD_THREADS_MAX" redefined
>> In file included from hsmspeed.c:33:
>> /usr/include/pthread.h:55:1: warning: this is the location of the
>> previous definition
>> ===
>>
>> Third and final (for now):
>> ===
>> hsmspeed.c: In function 'sign':
>> hsmspeed.c:120: warning: control reaches end of non-void function
>> ===
>>
>
> Jerry Lundström from the OpenDNSSEC-team has been kind enough to create
> a branch where he fixed the stuff I posted above, and made it into a pull
> request:
> https://github.com/opendnssec/opendnssec/pull/83
>
> I have imported the changes as patches and those warnings are now gone.
>
> Since they have been sorted out I'll now go through the the rest of the
> warnings. Since upstream is watching I will be as verbose as possible.
>
> First of all there are a bunch of libraries spitting out warnings on several
> locations, I consider these outside the scope:
>
> ===
> .libs/libxml2.so.15.1: warning: strcpy() is almost always misused, please use
> strlcpy()
> .libs/libxml2.so.15.1: warning: rand_r() isn't random; consider using
> arc4random()
> .libs/libldns.so.6.1: warning: random() isn't random; consider using
> arc4random()
> .libs/libxml2.so.15.1: warning: strcat() is almost always misused, please use
> strlcat()
> ===
They are indeed outside the scope of this port.
> Now for the first warning in the code at hand:
>
> ../ksm/libksm.a(datetime.o)(.text+0x562): In function `DtSecondsInterval':
> : warning: strcpy() is almost always misused, please use strlcpy()
>
> The code looks like this:
> enforcer/ksm/datetime.c:
> char buffer[64];
> [...]
> else {
> strcpy(buffer, "0s");
> }
>
> I am not sure trying to turn this into strlcpy is worth it since the
> string is static and the buffer obviously large enough. (Though I
> welcome cluebats from anyone more well versed in C here).
It is not about this particular occurrence. If you replace it by
a strlcpy call, you will probably see the same warning, coming from
another strcpy call elsewhere.
Propose upstream strl* and snprintf as alternatives to the unchecked
functions. If upstream sees interest in this, then a careful audit of
the code can follow, and strl* functions make error checking easier.
Adding fallback code for OSes not providing those functions is easy too.
> Next up some random warnings:
>
> ===
> ../ksm/libksm.a(ksm_policy.o)(.text+0xd31): In function `KsmPolicyUpdateSalt':
> : warning: rand() isn't random; consider using arc4random()
> ../ksm/libksm.a(ksm_policy.o)(.text+0xd13): In function `KsmPolicyUpdateSalt':
> : warning: srand() seed choices are invariably poor
> ===
>
> Now these warnings were interesting, looking into the code it looks like
> this:
>
> enforcer/ksm/ksm_policy.c:
> ===
> #ifdef HAVE_ARC4RANDOM
> for (i = 0; i < 2*(policy->denial->saltlength); i++) {
> salt[i] = hex_chars[arc4random()%strlen(hex_chars)];
> }
> #else
> srand( time(NULL) );
> for (i = 0; i < 2*(policy->denial->saltlength); i++) {
> salt[i] = hex_chars[rand()%strlen(hex_chars)];
> }
> #endif
> ===
>
> It is obviously prepared to use arc4random() if it is available, yet it does
> not for some reason.
>
> I found HAVE_ARC4RANDOM is indeed set:
> common/config.h:
> ===
> /* Define to 1 if you have the `arc4random' function. */
> #define HAVE_ARC4RANDOM 1
>
> /* Define to 1 if you have the `arc4random_uniform' function. */
> #define HAVE_ARC4RANDOM_UNIFORM 1
> ===
>
> And even -DHAVE_CONFIG_H is supplied during the build:
> ===
> cc -DHAVE_CONFIG_H -I. -I../../common -I../../common -I../../common
> -I./include -I./include -I/usr/local/include -I/usr/local/include/libxml2
> -I/usr/l
> ocal/include -O2 -pipe -Wall -Wextra -pthread -MT ksm_policy.o -MD -MP -MF
> .deps/ksm_policy.Tpo -c -o ksm_policy.o ksm_policy.c
> ===
>
> However, I finally noticed config.h is never included in ksm_policy.c. I tried
> doing so before the current #include lines as is done in other files, and it
> made the warning disappear.
Cool.
> There was one more random-related warning:
>
> ===
> notify.o(.text+0x233): In function `notify_setup':^M
> : warning: random() isn't random; consider using arc4random()^M
> ===
>
> The call in signer/src/wire/notify.c:
> random()%(extra-base);
>
> I guess #ifdef'ing this with HAVE_ARC4RANDOM like is done in ksm_policy.c
> would be nice.
See arc4random_uniform(3), the configure seems to test for its presence.
> Next up some strcat stuff:
>
> ===
> ../ksm/libksm.a(datetime.o)(.text+0x961): In function `DtAppendTime':
> : warning: strcat() is almost always misused, please use strlcat()
> ===
>
> The DtAppendTime() function in enforcer/ksm/datetime.c contains six
> calls to strcat, I dont feel comfortable converting these to strlcat, in
> case i introduce bugs myself. Input is welcome though.
>
> And another instance:
>
> ===
> kc_helper.o(.text+0x677): In function `StrAppend':
> : warning: strcat() is almost always misused, please use strlcat()
> ===
>
> ... the call in enforcer/utils/kc_helper.c:
> strcat(*str1, str2);
>
> Same thing applies here. Input is welcome.
Same reply as for strcpy.
> Finally some unused variables, might as well be complete while I am at
> it:
>
> ===
> daemon/cmdhandler.c:398: warning: unused variable 'task'
>
> shared/duration.c:442: warning: 'is_leap_year' defined but not used
> shared/duration.c:449: warning: 'leap_days' defined but not used
>
> shared/hsm.c:224: warning: unused variable 'retries'
> shared/hsm.c:220: warning: unused variable 'status'
> ===
Those could be harmless, or they could show a bug in the program logic.
If you want to audit them fine, but in the end it's rather upstream's
responsibility.
We try to avoid having too much patches in the ports tree. The best way
is probably to work with upstream and get your patches integrated, after
proper reviewing. Some mistakes have been done while wrongly replacing
eg. strcpy by strlcpy.
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
