cmake test failures on s390x Fedora 33 and 34

[mdomsch]

Direwolf 1.6 tag, building on s390x architecture on Fedora 33 and rawhide (will be F34) fails 'cmake test' with a buffer overflow detected. Build succeeds on s390x on Fedora 32, and on all other architectures of F32-34. Test #7 (enctest) has the failure.

Fedora rawhide (future F34) https://koji.fedoraproject.org/koji/buildinfo?buildID=1636157
Fedora 33 https://koji.fedoraproject.org/koji/buildinfo?buildID=1636158
Fedora 32 https://koji.fedoraproject.org/koji/buildinfo?buildID=1636159

While it's possible to ExcludeArch: s390x, and one could argue that no one sane would try running direwolf on a mainframe, it's often the case that these failures are latent on other architectures when detected on one.

From the build.log files of the rawhide build:

Executing(%check): /bin/sh -e /var/tmp/rpm-tmp.wGau2O

• umask 022
• cd /builddir/build/BUILD
• cd direwolf-1.6
• cd s390x-redhat-linux-gnu
• /usr/bin/ctest --output-on-failure --force-new-ctest-process -j3
  Test project /builddir/build/BUILD/direwolf-1.6/s390x-redhat-linux-gnu
  Start 1: dtest
  Start 2: ttest
  Start 3: tttexttest
  1/20 Test Font color similar to background #2: ttest ............................ Passed 0.00 sec
  Start 4: pftest
  2/20 Test Makefile.linux #3: tttexttest ....................... Passed 0.00 sec
  Start 5: tlmtest
  3/20 Test Added HAMLib support for PTT control #5: tlmtest .......................... Passed 0.00 sec
  Start 6: lltest
  4/20 Test audio_portaudio.c line 917, Abort trap : 6 #6: lltest ........................... Passed 0.00 sec
  Start 7: enctest
  5/20 Test GPIO pin naming #4: pftest ........................... Passed 0.00 sec
  Start 8: kisstest
  6/20 Test debian packaging #8: kisstest ......................... Passed 0.00 sec
  Start 9: pad2test
  7/20 Test The telemetry toolkit would benefit from a simple way to increase sequence numbers #9: pad2test ......................... Passed 0.00 sec
  Start 10: xidtest
  8/20 Test KISS TCP only accepts one client #7: enctest ..........................Child aborted***Exception: 0.80 sec
  *** buffer overflow detected ***: terminated
  Start 11: dtmftest
  9/20 Test Support for GPIO TX inhibit line #11: dtmftest ......................... Passed 0.02 sec
  Start 12: check-fx25
  10/20 Test Input support (TXINH so far) #12: check-fx25 ....................... Passed 0.01 sec
  Start 13: check-modem1200
  11/20 Test IGate Stats to Telemetry Data #1: dtest ............................ Passed 5.00 sec
  Start 14: check-modem300
  12/20 Test TX inhibit GPIO support #10: xidtest .......................... Passed 6.00 sec
  Start 15: check-modem9600
  13/20 Test Absolute Newbie with a question #15: check-modem9600 .................. Passed 0.38 sec
  Start 16: check-modem19200
  14/20 Test How to configure hamlib for ptt #16: check-modem19200 ................. Passed 0.48 sec
  Start 17: check-modem2400-a
  15/20 Test Merge pull request #1 from wb2osz/dev #13: check-modem1200 .................. Passed 7.42 sec
  Start 18: check-modem2400-b
  16/20 Test OS X packaging #17: check-modem2400-a ................ Passed 5.60 sec
  Start 19: check-modem2400-g
  17/20 Test Two direwolf stations transmit at same time (collide) how to adjust hold-off timer? #19: check-modem2400-g ................ Passed 0.30 sec
  Start 20: check-modem4800
  18/20 Test Can Direwolf jump into 14400 or 19200 baud rates #18: check-modem2400-b ................ Passed 4.84 sec
  19/20 Test Misc fixes for upstream #20: check-modem4800 .................. Passed 2.46 sec
  20/20 Test Fixes to Makefile.macosx #14: check-modem300 ................... Passed 11.99 sec
  95% tests passed, 1 tests failed out of 20
  Total Test time (real) = 17.00 sec
  The following tests FAILED:
  7 - enctest (Child aborted)
  Errors while running CTest
  error: Bad exit status from /var/tmp/rpm-tmp.wGau2O (%check)

[sharkcz]

@mdomsch, please ping me on IRC, if you want to look into it yourself. The same applies to upstream devels, I can provide access to a s390x machine. Just by the name enctest I would suspect some endianness bug ...

[wb2osz]

The only place where endianness should matter is in serializing integers with the AGW network protocol. There is already code in there to handle this. It's the same idea as the htonl() and similar functions except it has the opposite network byte order. It would be interesting to know why it fails. As you said it might be exposing some latent issue.

[mdomsch]

Reproduced on an s390x VM provided by @sharkcz.

Starting program: /var/lib/mock/fedora-rawhide-s390x-mdomsch/root/builddir/build/BUILD/direwolf-1.6/s390x-redhat-linux-gnu/test/enctest
Missing separate debuginfos, use: dnf debuginfo-install glibc-2.31-4.fc32.s390x
*** buffer overflow detected ***: terminated

Program received signal SIGABRT, Aborted.
0x000003fffdc4a426 in raise () from /lib64/libc.so.6
(gdb) bt
#0  0x000003fffdc4a426 in raise () from /lib64/libc.so.6
#1  0x000003fffdc2b00e in abort () from /lib64/libc.so.6
#2  0x000003fffdc910c0 in __libc_message () from /lib64/libc.so.6
#3  0x000003fffdd34094 in __fortify_fail () from /lib64/libc.so.6
#4  0x000003fffdd32798 in __chk_fail () from /lib64/libc.so.6
#5  0x000003fffdc87282 in _IO_str_chk_overflow () from /lib64/libc.so.6
#6  0x000003fffdc7c0f4 in __vfprintf_internal () from /lib64/libc.so.6
#7  0x000003fffdc87342 in __vsprintf_internal () from /lib64/libc.so.6
#8  0x000003fffdd3229a in __sprintf_chk@@GLIBC_2.4 () from /lib64/libc.so.6
#9  0x000002aa0000198c in sprintf (__fmt=0x2aa00007acc "%03d%s%c", __s=0x3ffffffecae "07126.47\357\070") at /builddir/build/BUILD/direwolf-1.6/src/latlong.c:145
#10 longitude_to_str (slong=0x3ffffffecae "07126.47\357\070", ambiguity=0, dlong=71.44116666666666) at /builddir/build/BUILD/direwolf-1.6/src/latlong.c:181
#11 set_norm_position (presult=0x3ffffffeca5, ambiguity=0, dlong=-71.44116666666666, dlat=42.576833333333333, symbol=38 '&', symtab=68 'D')
    at /builddir/build/BUILD/direwolf-1.6/src/encode_aprs.c:93
#12 encode_position (result_size=100, presult=0x3ffffffeca4 "!4234.61ND07126.47\357\070", comment=0x0, offset=0, tone=0, freq=0, speed=0, course=-999999, dir=0x0, gain=0, height=0, power=0,
    symbol=38 '&', symtab=68 'D', alt_ft=-999999, ambiguity=0, lon=-71.44116666666666, lat=42.576833333333333, compressed=0, messaging=0)
    at /builddir/build/BUILD/direwolf-1.6/src/encode_aprs.c:560
#13 main (argc=<optimized out>, argv=<optimized out>) at /builddir/build/BUILD/direwolf-1.6/src/encode_aprs.c:843

[mdomsch]

        snprintf (smin, sizeof(smin), "%05.2f", dmin);
        /* Due to roundoff, 59.9999 could come out as "60.00" */
        if (smin[0] == '6') {
          smin[0] = '0';
          ideg++;
        }

        sprintf (slong, "%03d%s%c", ideg, smin, hemi);

slong is 9 characters (from position_t). smin is 8 characters (from this function's stack). This format string can thus be 3 + 8 + 1 + null terminator = 13 characters long, trying to fit in a 9-char buffer.

Shouldn't the formatting of smin be %02.2f instead of %05.2f ? You can't have more than 60 minutes. That saves 3 characters, which would then fit (except for the null terminator).

I tried changing this to %02.2f, and it still fails.

[mdomsch]

Are the lat and lon fields in position_t supposed to be null-terminated strings, or an array of char with no null terminator? If the latter, then doing the sprintf into a temporary (stack) buffer of say size 16, then memcpy from the buffer into each of lat and lon just the length of lat and long, does succeed and lets all the tests pass. Not pretty, but it works.

diff --git a/src/latlong.c b/src/latlong.c
index b3eadcc..1de9c5a 100644
--- a/src/latlong.c
+++ b/src/latlong.c
@@ -78,6 +78,7 @@ void latitude_to_str (double dlat, int ambiguity, char *slat)
        int ideg;       /* whole number of degrees. */
        double dmin;    /* Minutes after removing degrees. */
        char smin[8];   /* Minutes in format mm.mm */
+       char tempbuf[16];

        if (dlat < -90.) {
          text_color_set(DW_COLOR_ERROR);
@@ -101,14 +102,17 @@ void latitude_to_str (double dlat, int ambiguity, char *slat)
        ideg = (int)dlat;
        dmin = (dlat - ideg) * 60.;

-       snprintf (smin, sizeof(smin), "%05.2f", dmin);
+       snprintf (smin, sizeof(smin), "%02.2f", dmin);
        /* Due to roundoff, 59.9999 could come out as "60.00" */
        if (smin[0] == '6') {
          smin[0] = '0';
          ideg++;
        }

-       sprintf (slat, "%02d%s%c", ideg, smin, hemi);
+       text_color_set(DW_COLOR_DEBUG);
+       dw_printf("latitude_to_str ideg=%d smin=%s hemi=%c\n", ideg, smin, hemi);
+       snprintf(tempbuf, sizeof(tempbuf), "%02d%s%c", ideg, smin, hemi);
+       memcpy(slat, tempbuf, 8);

        if (ambiguity >= 1) {
          slat[6] = ' ';
@@ -148,6 +152,7 @@ void longitude_to_str (double dlong, int ambiguity, char *slong)
        int ideg;       /* whole number of degrees. */
        double dmin;    /* Minutes after removing degrees. */
        char smin[8];   /* Minutes in format mm.mm */
+       char tempbuf[16];

        if (dlong < -180.) {
          text_color_set(DW_COLOR_ERROR);
@@ -171,14 +176,17 @@ void longitude_to_str (double dlong, int ambiguity, char *slong)
        ideg = (int)dlong;
        dmin = (dlong - ideg) * 60.;

-       snprintf (smin, sizeof(smin), "%05.2f", dmin);
+       snprintf (smin, sizeof(smin), "%02.2f", dmin);
        /* Due to roundoff, 59.9999 could come out as "60.00" */
        if (smin[0] == '6') {
          smin[0] = '0';
          ideg++;
        }

-       sprintf (slong, "%03d%s%c", ideg, smin, hemi);
+       text_color_set(DW_COLOR_DEBUG);
+       dw_printf ("longitude_to_str ideg=%d smin=%s hemi=%c\n", ideg, smin, hemi);
+       sprintf (tempbuf, "%03d%s%c", ideg, smin, hemi);
+       memcpy(slong, tempbuf, 9);
 /*
  * The spec says position ambiguity in latitude also
  * applies to longitude automatically.

[mdomsch]

While that worked on s390x and armv7hl, the lltest executable fails all tests on all the other architectures including i686 and x86_64. It clearly needed the null terminators. So, back to the drawing board.

 5/20 Test  #6: lltest ...........................***Failed    0.01 sec
Error 1.1: Did not expect "4515.00N��
�$V"
Error 1.2: Did not expect "4515.00S��
�$V"
Error 1.3: Did not expect "4559.99N��
�$V"
Error 1.4: Did not expect "4600.00N��
�$V"
Error 1.5: Did not expect "4559.9 N��
�$V"
Error 1.6: Did not expect "4559.  N��
�$V"
Error 1.7: Did not expect "455 .  N��
�$V"
Error 1.8: Did not expect "45  .  N��
�$V"
Error 2.1: Did not expect "04515.00E�
�$V"
Error 2.2: Did not expect "04515.00W�
�$V"
Error 2.3: Did not expect "04559.99E�
�$V"
Error 2.4: Did not expect "04600.00E�
�$V"
Error 2.5: Did not expect "04559.9 E�
�$V"
Error 2.6: Did not expect "04559.  E�
�$V"
Error 2.7: Did not expect "0455 .  E�
�$V"
Error 2.8: Did not expect "045  .  E�
�$V"

[wb2osz]

Fixed in dev branch. 4a1aa2b
Is this good enough or do you want it merged to master branch?

[mdomsch]

This is good enough, thank you very much! I will build Fedora 32, 33, and rawhide packages tonight, should hit the updates-testing tree in a couple days and the updates-released trees in a couple weeks.

…

On Thu, Nov 5, 2020, 5:53 PM wb2osz ***@***.***> wrote: Fixed in dev branch. 4a1aa2b <4a1aa2b> Is this good enough or do you want it merged to master branch? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#296 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFIHKUL3CAS6BP5N5PQ5BTSOM3JDANCNFSM4TESIFOQ> .

[wb2osz]

Fixed.