clang AddressSanitizer: global-buffer-overflow in map_dos.c map_plot()

[godfreja]

While running clangs -fsanitize=address feature an error is encountered with worldhi.map enabled:

==99429==ERROR: AddressSanitizer: global-buffer-overflow on address 0x00010440805c at pc 0x0001040dfb17 bp 0x7ffeebc7edd0 sp 0x7ffeebc7edc8
READ of size 2 at 0x00010440805c thread T0
    #0 0x1040dfb16 in map_plot map_dos.c:316
    #1 0x1040e275b in draw_dos_map map_dos.c:1011
    #2 0x1040d17fc in draw_map maps.c:5796
    #3 0x1040d9da4 in load_maps maps.c:8985
    #4 0x1040554b0 in create_image main.c:3334
    #5 0x104050675 in new_image main.c:13019
    #6 0x1040b9505 in UpdateTime main.c:11422
    #7 0x10493a19d in XtAppProcessEvent (libXt.6.dylib:x86_64+0x1919d)
    #8 0x10492f636 in XtAppMainLoop (libXt.6.dylib:x86_64+0xe636)
    #9 0x1040b8352 in main main.c:27517
    #10 0x7fff6f8d1084 in start (libdyld.dylib:x86_64+0x17084)

0x00010440805c is located 4 bytes to the left of global variable 'points' defined in '../../Xastir.github/src/map_dos.c:114:19' (0x104408060) of size 400000
0x00010440805c is located 56 bytes to the right of global variable 'redraw_check' defined in '../../Xastir.github/src/map_dos.c:113:16' (0x104408020) of size 4
SUMMARY: AddressSanitizer: global-buffer-overflow map_dos.c:316 in map_plot
Shadow bytes around the buggy address:
  0x100020880fb0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x100020880fc0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x100020880fd0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x100020880fe0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x100020880ff0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
=>0x100020881000: f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9[f9]00 00 00 00
  0x100020881010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100020881020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100020881030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100020881040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100020881050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==99429==ABORTING
Magick: abort due to signal 6 (SIGABRT) "Abort"...
Abort trap: 6

The relevant chunk of code is:

        if (points[npoints].x != points[npoints - 1].x || points[npoints].y != points[npoints - 1].y) {
            if (last_behavior & 0x80) {
                npoints++;
            }
            else if (points[npoints].x > (-MAX_OUTBOUND)
                     && points[npoints].x < (short)max_x
                     && points[npoints].y > (-MAX_OUTBOUND)
                     && points[npoints].y < (short)max_y) {
                npoints++;
            }
        }

In this case npoints is 0 so the points [npoints - 1] expression reads outside the array. I'll need to study the code for a bit in order to figure out what an appropriate fix would be. I may move on and see if there are other errors I can encounter first.

To run with this clang feature enabled I ran configure with CFLAGS="-O1 -g -fsanitize=address -fno-omit-frame-pointer"

[we7u]

Is it possible to add the CFLAGS to our OSX builds on Travis-CI? Maybe that wouldn't make sense long-term on a CI engine, but more of a once-a-year trial by one of the developers to catch anything introduced by new code?

[godfreja]

Unfortunately you need to actually run the program to see anything resulting from adding the flags, so doing it in Travis doesn't really get us anything.

It would be nice if there was some way to take advantage of this in an automated way. I'm working on another bug it found where we were doing a 200 byte fill on a 100 byte array. I've been thinking about having some automated tests for xastir would be a nice addition for travis-ci. Given that xastir is a X11 program there are chunks that would probably be hard to test, but we could maybe starting working on more standalone pieces.

[we7u]

Trying another fix. This one looks better:

    if (npoints == 0) { // First point drawn in the line
        if (points[npoints].x > (-MAX_OUTBOUND)
                && points[npoints].x < (short)max_x
                && points[npoints].y > (-MAX_OUTBOUND)
                && points[npoints].y < (short)max_y) {
            npoints++;  
        }
    }
    else {
        if (points[npoints].x != points[npoints - 1].x || points[npoints].y != points[npoints - 1].y) {
            if (last_behavior & 0x80) {
                npoints++;
            }
            else if (points[npoints].x > (-MAX_OUTBOUND)
                     && points[npoints].x < (short)max_x
                     && points[npoints].y > (-MAX_OUTBOUND)
                     && points[npoints].y < (short)max_y) {
                npoints++;
            }
        }
    }

[we7u]

FYI: "npoints-1" is used several times in maps.c, and "npoints - 1" is used in maps.c and map_dos.c. Similar fixes may need to be done for maps.c.

[godfreja]

I haven't looked at the mapping code before (besides filing this bug) so I need to look at it for a bit before I feel qualified to comment. (Which is why I haven't done anything with this bug yet.) If Tom is OK with your changes before I have time to understand what is happening here I'll defer to him.

[tvrusso]

I do not understand this code, either, and am not prepared to sign off on it. It's not at all clear what it's trying to do.

[we7u]

Fun isn't it? I think that was one of the original bits of map code, before I joined the project in 1999. Very few comments. I _think_ there are limits on how long a vector is supposed to be and they're using "npoints" to determine that. Add to that the map viewport which might be a subset of the map we're drawing or might be larger than the map we're drawing, so we check whether we're trying to draw inside or outside of the viewport (and don't draw if it's outside). I see a comparison: "if (npoints > MAX_MAP_POINTS)" in one spot. Looks like "npoints" is used in map_dos.c, map_shp.c, map_tif.c, and maps.c. I must have fully understood it at some point 'cuz my first entry into the Xastir project was writing map_tif.c from scratch.

…

On Tue, May 7, 2019 at 2:47 PM Tom Russo ***@***.***> wrote: I do not understand this code, either, and am not prepared to sign off on it. It's not at all clear what it's trying to do. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#73 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEMNKX66K56ODJ5VNT3U7QLPUH2GLANCNFSM4HHTMCZQ> .

-- Curt, WE7U http://we7u.wetnet.net http://www.sarguydigital.com

[tvrusso]

Yes, git log shows that the code in question has been present in maps.c since it was imported into sourceforge CVS in commit fa09e55.

[godfreja]

I wonder if we should defer this until after the next release since it is a long standing bug and the fix is not obvious.

[we7u]

+1

…

On Wed, May 8, 2019 at 7:25 AM Jason Godfrey ***@***.***> wrote: I wonder if we should defer this until after the next release since it is a long standing bug and the fix is not obvious. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#73 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEMNKX36GHAHHEAGPWORFKLPULPFJANCNFSM4HHTMCZQ> .

-- Curt, WE7U http://we7u.wetnet.net http://www.sarguydigital.com

[tvrusso]

I don't wonder, I think it's a sleeping dog we should let lie for a few more weeks. Let's just move it off the ToDo for this release in the Kanban.