From: Dave Hart Date: Thu, 14 Apr 2011 02:00:02 +0000 (+0000) Subject: Detect vsnprintf() support for "%m" and disable our "%m" expansion. X-Git-Tag: NTP_4_2_7P151~2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=46f6a8ee7ac57d05b1fd50901af619cd6e39a909;p=thirdparty%2Fntp.git Detect vsnprintf() support for "%m" and disable our "%m" expansion. Add --enable-c99-sprintf to configure args for -noopenssl variety of flock-build to avoid regressions in (v)snprintf() replacement. More msnprintf() unit tests. Coverity Scan error checking fixes. bk: 4da65522l3kIEGfqELWnvARUQGzmuQ --- diff --git a/ChangeLog b/ChangeLog index 915fbb421..f2e1ba486 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,8 @@ +* Detect vsnprintf() support for "%m" and disable our "%m" expansion. +* Add --enable-c99-sprintf to configure args for -noopenssl variety of + flock-build to avoid regressions in (v)snprintf() replacement. +* More msnprintf() unit tests. +* Coverity Scan error checking fixes. (4.2.7p150) 2011/04/13 Released by Harlan Stenn * Remove never-used, incomplete ports/winnt/ntpd/refclock_trimbledc.[ch] * On systems without C99-compliant (v)snprintf(), use C99-snprintf diff --git a/flock-build b/flock-build index 926225058..69f7ffe80 100755 --- a/flock-build +++ b/flock-build @@ -81,7 +81,7 @@ do 0) ssh $i "cd $c_d ; ./build $SIG $PARSE $STD $BUILD_ARGS" & ssh $i "cd $c_d ; ./build $SIG $PARSE $STD --disable-debugging $BUILD_ARGS" & - ssh $i "cd $c_d ; ./build $SIG $PARSE $STD --without-crypto $BUILD_ARGS" & + ssh $i "cd $c_d ; ./build $SIG $PARSE $STD --without-crypto --enable-c99-snprintf $BUILD_ARGS" & ssh $i "cd $c_d ; ./build $SIG $STD --disable-all-clocks --disable-autokey --without-sntp --disable-thread-support $BUILD_ARGS" & ;; 1) @@ -109,7 +109,7 @@ do echo \`date -u '+%H:%M:%S'\` $i started build \$COUNT of 4 [ 0 -lt \`expr \$COUNT % $PARALLEL_BUILDS\` ] || wait - ./build $SIG $PARSE $STD --without-crypto $BUILD_ARGS & + ./build $SIG $PARSE $STD --without-crypto --enable-c99-snprintf $BUILD_ARGS & COUNT=\`expr \$COUNT + 1\` echo \`date -u '+%H:%M:%S'\` $i started build \$COUNT of 4 diff --git a/libntp/audio.c b/libntp/audio.c index 5fbe23902..766d17fdd 100644 --- a/libntp/audio.c +++ b/libntp/audio.c @@ -174,20 +174,20 @@ audio_config_read( for (; *ca && isascii((int)*ca) && (isspace((int)*ca) || (*ca == '=')); ca++) continue; - if (!strncmp(cc, "IDEV", (size_t) 4)) { - sscanf(ca, "%99s", ab); + if (!strncmp(cc, "IDEV", 4) && + 1 == sscanf(ca, "%99s", ab)) { strlcpy(cf_i_dev, ab, sizeof(cf_i_dev)); printf("idev <%s>\n", ab); - } else if (!strncmp(cc, "CDEV", (size_t) 4)) { - sscanf(ca, "%99s", ab); + } else if (!strncmp(cc, "CDEV", 4) && + 1 == sscanf(ca, "%99s", ab)) { strlcpy(cf_c_dev, ab, sizeof(cf_c_dev)); printf("cdev <%s>\n", ab); - } else if (!strncmp(cc, "AGC", (size_t) 3)) { - sscanf(ca, "%99s", ab); + } else if (!strncmp(cc, "AGC", 3) && + 1 == sscanf(ca, "%99s", ab)) { strlcpy(cf_agc, ab, sizeof(cf_agc)); printf("agc <%s> %d\n", ab, i); - } else if (!strncmp(cc, "MONITOR", (size_t) 7)) { - sscanf(ca, "%99s", ab); + } else if (!strncmp(cc, "MONITOR", 7) && + 1 == sscanf(ca, "%99s", ab)) { strlcpy(cf_monitor, ab, sizeof(cf_monitor)); printf("monitor <%s> %d\n", ab, mixer_name(ab, -1)); } diff --git a/libntp/msyslog.c b/libntp/msyslog.c index f2c38ed1d..18814b5c9 100644 --- a/libntp/msyslog.c +++ b/libntp/msyslog.c @@ -38,14 +38,15 @@ extern char * progname; /* Declare the local functions */ void addto_syslog (int, const char *); +#ifndef VSNPRINTF_PERCENT_M void format_errmsg (char *, size_t, const char *, int); /* * Work around misdetection by AC_FUNC_STRERROR_R on Debian Linux. */ -#if defined(STRERROR_R_CHAR_P) && strerror_r == __xpg_strerror_r -# undef STRERROR_R_CHAR_P -#endif +# if defined(STRERROR_R_CHAR_P) && strerror_r == __xpg_strerror_r +# undef STRERROR_R_CHAR_P +# endif /* @@ -54,7 +55,7 @@ void format_errmsg (char *, size_t, const char *, int); * For Windows, we have: * #define errno_to_str isc_strerror */ -#ifndef errno_to_str +# ifndef errno_to_str void errno_to_str( int err, @@ -62,40 +63,82 @@ errno_to_str( size_t bufsiz ) { -# if defined(STRERROR_R_CHAR_P) || !HAVE_DECL_STRERROR_R +# if defined(STRERROR_R_CHAR_P) || !HAVE_DECL_STRERROR_R char * pstatic; buf[0] = '\0'; -# ifdef STRERROR_R_CHAR_P +# ifdef STRERROR_R_CHAR_P /* * For older GNU strerror_r, the return value either points to * buf, or to static storage. We want the result always in buf */ pstatic = strerror_r(err, buf, bufsiz); -# else +# else pstatic = strerror(err); -# endif +# endif if (NULL == pstatic && '\0' == buf[0]) snprintf(buf, bufsiz, "%s(%d): errno %d", -# ifdef STRERROR_R_CHAR_P +# ifdef STRERROR_R_CHAR_P "strerror_r", -# else +# else "strerror", -# endif +# endif err, errno); /* protect against believing an int return is a pointer */ else if (pstatic != buf && pstatic > (char *)bufsiz) strlcpy(buf, pstatic, bufsiz); -# else +# else int rc; rc = strerror_r(err, buf, bufsiz); if (rc < 0) snprintf(buf, bufsiz, "strerror_r(%d): errno %d", err, errno); -# endif +# endif } -#endif /* errno_to_str */ +# endif /* errno_to_str */ + + +void +format_errmsg( + char * nfmt, + size_t lennfmt, + const char * fmt, + int errval + ) +{ + char errmsg[256]; + char c; + char *n; + const char *f; + size_t len; + + n = nfmt; + f = fmt; + while ((c = *f++) != '\0' && n < (nfmt + lennfmt - 1)) { + if (c != '%') { + *n++ = c; + continue; + } + if ((c = *f++) != 'm') { + *n++ = '%'; + if ('\0' == c) + break; + *n++ = c; + continue; + } + errno_to_str(errval, errmsg, sizeof(errmsg)); + len = strlen(errmsg); + + /* Make sure we have enough space for the error message */ + if ((n + len) < (nfmt + lennfmt - 1)) { + memcpy(n, errmsg, len); + n += len; + } + } + *n = '\0'; +} +#endif /* VSNPRINTF_PERCENT_M */ /* @@ -173,47 +216,6 @@ addto_syslog( } -void -format_errmsg( - char * nfmt, - size_t lennfmt, - const char * fmt, - int errval - ) -{ - char errmsg[256]; - char c; - char *n; - const char *f; - size_t len; - - n = nfmt; - f = fmt; - while ((c = *f++) != '\0' && n < (nfmt + lennfmt - 1)) { - if (c != '%') { - *n++ = c; - continue; - } - if ((c = *f++) != 'm') { - *n++ = '%'; - if ('\0' == c) - break; - *n++ = c; - continue; - } - errno_to_str(errval, errmsg, sizeof(errmsg)); - len = strlen(errmsg); - - /* Make sure we have enough space for the error message */ - if ((n + len) < (nfmt + lennfmt - 1)) { - memcpy(n, errmsg, len); - n += len; - } - } - *n = '\0'; -} - - int mvsnprintf( char * buf, @@ -222,9 +224,10 @@ mvsnprintf( va_list ap ) { +#ifndef VSNPRINTF_PERCENT_M char nfmt[256]; +#endif int errval; - int rc; /* * Save the error value as soon as possible @@ -235,13 +238,12 @@ mvsnprintf( #endif /* SYS_WINNT */ errval = errno; +#ifndef VSNPRINTF_PERCENT_M format_errmsg(nfmt, sizeof(nfmt), fmt, errval); - - rc = vsnprintf(buf, bufsiz, nfmt, ap); - /* terminate buf, as MS vsnprintf() does not when truncating */ - buf[bufsiz - 1] = '\0'; - - return rc; +#else + errno = errval; +#endif + return vsnprintf(buf, bufsiz, nfmt, ap); } @@ -252,9 +254,10 @@ mvfprintf( va_list ap ) { +#ifndef VSNPRINTF_PERCENT_M char nfmt[256]; +#endif int errval; - int rc; /* * Save the error value as soon as possible @@ -265,10 +268,12 @@ mvfprintf( #endif /* SYS_WINNT */ errval = errno; +#ifndef VSNPRINTF_PERCENT_M format_errmsg(nfmt, sizeof(nfmt), fmt, errval); - rc = vfprintf(fp, nfmt, ap); - - return rc; +#else + errno = errval; +#endif + return vfprintf(fp, nfmt, ap); } diff --git a/libntp/snprintf.c b/libntp/snprintf.c index 4ea9aed66..9a72eb2de 100644 --- a/libntp/snprintf.c +++ b/libntp/snprintf.c @@ -803,6 +803,7 @@ rpl_vsnprintf(char *str, size_t size, const char *format, va_list args) /* FALLTHROUGH */ case 'F': flags |= PRINT_F_UP; + /* FALLTHROUGH */ case 'a': /* Not yet supported, we'll use "%f". */ /* FALLTHROUGH */ diff --git a/ntpd/refclock_arc.c b/ntpd/refclock_arc.c index 8ac58f656..b11854393 100644 --- a/ntpd/refclock_arc.c +++ b/ntpd/refclock_arc.c @@ -627,51 +627,49 @@ arc_start( { register struct arcunit *up; struct refclockproc *pp; + int temp_fd; int fd; char device[20]; #ifdef HAVE_TERMIOS struct termios arg; #endif - msyslog(LOG_NOTICE, "ARCRON: %s: opening unit %d", arc_version, unit); -#ifdef DEBUG - if(debug) { - printf("arc: %s: attempt to open unit %d.\n", arc_version, unit); - } -#endif - - /* Prevent a ridiculous device number causing overflow of device[]. */ - if((unit < 0) || (unit > 255)) { return(0); } + msyslog(LOG_NOTICE, "MSF_ARCRON %s: opening unit %d", + arc_version, unit); + DPRINTF(1, ("arc: %s: attempt to open unit %d.\n", arc_version, + unit)); /* * Open serial port. Use CLK line discipline, if available. */ snprintf(device, sizeof(device), DEVICE, unit); - fd = refclock_open(device, SPEED, LDISC_CLK); - if (fd <= 0) - return(0); -#ifdef DEBUG - if(debug) { printf("arc: unit %d using open().\n", unit); } -#endif + temp_fd = refclock_open(device, SPEED, LDISC_CLK); + if (temp_fd <= 0) + return 0; + DPRINTF(1, ("arc: unit %d using tty_open().\n", unit)); fd = tty_open(device, OPEN_FLAGS, 0777); - if(fd < 0) { -#ifdef DEBUG - if(debug) { printf("arc: failed [tty_open()] to open %s.\n", device); } -#endif - return(0); + if (fd < 0) { + msyslog(LOG_ERR, "MSF_ARCRON(%d): failed second open(%s, 0777): %m.\n", + unit, device); + close(temp_fd); + return 0; } + close(temp_fd); + temp_fd = -1; #ifndef SYS_WINNT fcntl(fd, F_SETFL, 0); /* clear the descriptor flags */ #endif -#ifdef DEBUG - if(debug) - { printf("arc: opened RS232 port with file descriptor %d.\n", fd); } -#endif + DPRINTF(1, ("arc: opened RS232 port with file descriptor %d.\n", fd)); #ifdef HAVE_TERMIOS - tcgetattr(fd, &arg); + if (tcgetattr(fd, &arg) < 0) { + msyslog(LOG_ERR, "MSF_ARCRON(%d): tcgetattr(%s): %m.\n", + unit, device); + close(fd); + return 0; + } arg.c_iflag = IGNBRK | ISTRIP; arg.c_oflag = 0; @@ -680,11 +678,16 @@ arc_start( arg.c_cc[VMIN] = 1; arg.c_cc[VTIME] = 0; - tcsetattr(fd, TCSANOW, &arg); + if (tcsetattr(fd, TCSANOW, &arg) < 0) { + msyslog(LOG_ERR, "MSF_ARCRON(%d): tcsetattr(%s): %m.\n", + unit, device); + close(fd); + return 0; + } #else - msyslog(LOG_ERR, "ARCRON: termios not supported in this driver"); + msyslog(LOG_ERR, "ARCRON: termios required by this driver"); (void)close(fd); return 0; diff --git a/ntpq/ntpq-subs.c b/ntpq/ntpq-subs.c index 446c9da9c..405b193ad 100644 --- a/ntpq/ntpq-subs.c +++ b/ntpq/ntpq-subs.c @@ -1590,7 +1590,9 @@ doprintpeers( while (nextvar(&datalen, &data, &name, &value)) { if (!strcmp("srcadr", name) || !strcmp("peeradr", name)) { - decodenetnum(value, &srcadr); + if (!decodenetnum(value, &srcadr)) + fprintf(stderr, "malformed %s=%s\n", + name, value); } else if (!strcmp("srchost", name)) { if (pvl == peervarlist) { len = strlen(value); @@ -3238,12 +3240,17 @@ collect_display_vdc( break; case NTP_ADP: - decodenetnum(val, &pvdc->v.sau); + if (!decodenetnum(val, &pvdc->v.sau)) + fprintf(stderr, "malformed %s=%s\n", + pvdc->tag, val); break; case NTP_ADD: if (0 == n) { /* adr */ - decodenetnum(val, &pvdc->v.sau); + if (!decodenetnum(val, &pvdc->v.sau)) + fprintf(stderr, + "malformed %s=%s\n", + pvdc->tag, val); } else { /* port */ if (atouint(val, &ul)) SET_PORT(&pvdc->v.sau, diff --git a/ntpq/ntpq.c b/ntpq/ntpq.c index 0bf881629..a3ab1b9fb 100644 --- a/ntpq/ntpq.c +++ b/ntpq/ntpq.c @@ -963,9 +963,9 @@ getresponse( } TRACE(2, ("Got packet, size = %d\n", n)); - if ((int)count > (n - CTL_HEADER_LEN)) { - TRACE(1, ("Received count of %u octets, data in packet is %d\n", - count, (n - CTL_HEADER_LEN))); + if ((long)count > (n - CTL_HEADER_LEN)) { + TRACE(1, ("Received count of %u octets, data in packet is %ld\n", + count, (long)n - CTL_HEADER_LEN)); continue; } if (count == 0 && CTL_ISMORE(rpkt.r_m_e_op)) { diff --git a/parseutil/dcfd.c b/parseutil/dcfd.c index 208fdd9c1..5c446fc69 100644 --- a/parseutil/dcfd.c +++ b/parseutil/dcfd.c @@ -587,8 +587,8 @@ cvt_rawdcf( /* * invalid character (no consecutive bit sequence) */ - dprintf(("parse: cvt_rawdcf: character check for 0x%x@%d FAILED\n", - (u_int)*s, s - buffer)); + dprintf(("parse: cvt_rawdcf: character check for 0x%x@%ld FAILED\n", + (u_int)*s, (long)(s - buffer))); *s = (unsigned char)~0; rtc = CVT_FAIL|CVT_BADFMT; } diff --git a/sntp/m4/ntp_libntp.m4 b/sntp/m4/ntp_libntp.m4 index dbf43dfe0..9c2f9e618 100644 --- a/sntp/m4/ntp_libntp.m4 +++ b/sntp/m4/ntp_libntp.m4 @@ -506,6 +506,68 @@ AC_DEFUN([NTP_C99_SNPRINTF], [ NTP_C99_SNPRINTF +dnl C99-snprintf does not handle %m +case "$hw_use_rpl_vsnprintf:$hw_cv_func_vsnprintf" in + no:yes) + AC_CACHE_CHECK( + [if vsnprintf expands "%m" to strerror(errno)], + [ntp_cv_vsnprintf_percent_m], + [AC_RUN_IFELSE( + [AC_LANG_PROGRAM( + [[ + #include + #include + #include + #include + + int call_vsnprintf( + char * dst, + size_t sz, + const char *fmt, + ... + ); + + int call_vsnprintf( + char * dst, + size_t sz, + const char *fmt, + ... + ) + { + va_list ap; + int rc; + + va_start(ap, fmt); + rc = vsnprintf(dst, sz, fmt, ap); + va_end(ap); + + return rc; + } + ]], + [[ + char sbuf[512]; + char pbuf[512]; + int slen; + + strcpy(sbuf, strerror(ENOENT)); + errno = ENOENT; + slen = call_vsnprintf(pbuf, sizeof(pbuf), "%m", + "wrong"); + return strcmp(sbuf, pbuf); + ]] + )], + [ntp_cv_vsnprintf_percent_m=yes], + [ntp_cv_vsnprintf_percent_m=no], + [ntp_cv_vsnprintf_percent_m=no] + )] + ) + case "$ntp_cv_vsnprintf_percent_m" in + yes) + AC_DEFINE([VSNPRINTF_PERCENT_M], [1], + [vsnprintf expands "%m" to strerror(errno)]) + esac +esac + AC_CHECK_HEADERS([sys/clockctl.h]) AC_ARG_ENABLE( diff --git a/tests/libntp/msyslog.cpp b/tests/libntp/msyslog.cpp index eddcfc50f..5b00703e6 100644 --- a/tests/libntp/msyslog.cpp +++ b/tests/libntp/msyslog.cpp @@ -4,8 +4,10 @@ extern "C" { #include #include #include +#ifndef VSNPRINTF_PERCENT_M // format_errmsg() is normally private to msyslog.c void format_errmsg (char *, size_t, const char *, int); +#endif }; class msyslogTest : public libntptest { @@ -15,46 +17,59 @@ class msyslogTest : public libntptest { TEST_F(msyslogTest, msnprintf) { #define FMT_PREFIX "msyslog.cpp ENOENT: " - char exp_buf[512]; - char act_buf[512]; + char exp_buf[512]; + char act_buf[512]; + int exp_cnt; + int act_cnt; - snprintf(exp_buf, sizeof(exp_buf), FMT_PREFIX "%s", - strerror(ENOENT)); + exp_cnt = snprintf(exp_buf, sizeof(exp_buf), FMT_PREFIX "%s", + strerror(ENOENT)); errno = ENOENT; - msnprintf(act_buf, sizeof(act_buf), FMT_PREFIX "%m"); + act_cnt = msnprintf(act_buf, sizeof(act_buf), FMT_PREFIX "%m"); + EXPECT_EQ(exp_cnt, act_cnt); EXPECT_STREQ(exp_buf, act_buf); } TEST_F(msyslogTest, msnprintfLiteralPercentm) { - char exp_buf[32]; - char act_buf[32]; + char exp_buf[32]; + char act_buf[32]; + int exp_cnt; + int act_cnt; - snprintf(exp_buf, sizeof(exp_buf), "%%m"); + exp_cnt = snprintf(exp_buf, sizeof(exp_buf), "%%m"); errno = ENOENT; - msnprintf(act_buf, sizeof(act_buf), "%%m"); + act_cnt = msnprintf(act_buf, sizeof(act_buf), "%%m"); + EXPECT_EQ(exp_cnt, act_cnt); EXPECT_STREQ(exp_buf, act_buf); } TEST_F(msyslogTest, msnprintfBackslashLiteralPercentm) { - char exp_buf[32]; - char act_buf[32]; + char exp_buf[32]; + char act_buf[32]; + int exp_cnt; + int act_cnt; - snprintf(exp_buf, sizeof(exp_buf), "\%%m"); + exp_cnt = snprintf(exp_buf, sizeof(exp_buf), "\%%m"); errno = ENOENT; - msnprintf(act_buf, sizeof(act_buf), "\%%m"); + act_cnt = msnprintf(act_buf, sizeof(act_buf), "\%%m"); + EXPECT_EQ(exp_cnt, act_cnt); EXPECT_STREQ(exp_buf, act_buf); } TEST_F(msyslogTest, msnprintfBackslashPercent) { - char exp_buf[32]; - char act_buf[32]; + char exp_buf[32]; + char act_buf[32]; + int exp_cnt; + int act_cnt; - snprintf(exp_buf, sizeof(exp_buf), "\%s", strerror(ENOENT)); + exp_cnt = snprintf(exp_buf, sizeof(exp_buf), "\%s", + strerror(ENOENT)); errno = ENOENT; - msnprintf(act_buf, sizeof(act_buf), "\%m"); + act_cnt = msnprintf(act_buf, sizeof(act_buf), "\%m"); + EXPECT_EQ(exp_cnt, act_cnt); EXPECT_STREQ(exp_buf, act_buf); } @@ -63,23 +78,60 @@ TEST_F(msyslogTest, msnprintfHangingPercent) static char fmt[] = "percent then nul term then non-nul %\0oops!"; char exp_buf[64]; char act_buf[64]; + int exp_cnt; + int act_cnt; - memset(exp_buf, 0, sizeof(exp_buf)); - memset(act_buf, 0, sizeof(act_buf)); - snprintf(exp_buf, sizeof(exp_buf), fmt); - msnprintf(act_buf, sizeof(act_buf), fmt); + ZERO(exp_buf); + ZERO(act_buf); + exp_cnt = snprintf(exp_buf, sizeof(exp_buf), fmt); + act_cnt = msnprintf(act_buf, sizeof(act_buf), fmt); + EXPECT_EQ(exp_cnt, act_cnt); EXPECT_STREQ(exp_buf, act_buf); EXPECT_STREQ("", act_buf + 1 + strlen(act_buf)); } +#ifndef VSNPRINTF_PERCENT_M TEST_F(msyslogTest, format_errmsgHangingPercent) { static char fmt[] = "percent then nul term then non-nul %\0oops!"; char act_buf[64]; - memset(act_buf, 0, sizeof(act_buf)); + ZERO(act_buf); format_errmsg(act_buf, sizeof(act_buf), fmt, ENOENT); EXPECT_STREQ(fmt, act_buf); EXPECT_STREQ("", act_buf + 1 + strlen(act_buf)); } +#endif +TEST_F(msyslogTest, msnprintfNullTarget) +{ + int exp_cnt; + int act_cnt; + + exp_cnt = snprintf(NULL, 0, "%d", 123); + errno = ENOENT; + act_cnt = msnprintf(NULL, 0, "%d", 123); + EXPECT_EQ(exp_cnt, act_cnt); +} + +TEST_F(msyslogTest, msnprintfTruncate) +{ + char undist[] = "undisturbed"; + char exp_buf[512]; + char act_buf[512]; + int exp_cnt; + int act_cnt; + + memcpy(exp_buf + 3, undist, sizeof(undist)); + memcpy(act_buf + 3, undist, sizeof(undist)); + exp_cnt = snprintf(exp_buf, 3, "%s", strerror(ENOENT)); + errno = ENOENT; + act_cnt = msnprintf(act_buf, 3, "%m"); + EXPECT_EQ('\0', exp_buf[2]); + EXPECT_EQ('\0', act_buf[2]); + EXPECT_TRUE(act_cnt > 0); + EXPECT_EQ(exp_cnt, act_cnt); + EXPECT_STREQ(exp_buf, act_buf); + EXPECT_STREQ(exp_buf + 3, undist); + EXPECT_STREQ(act_buf + 3, undist); +}