Use array delete for point arrays

[adenyes]

This change uses array delete on PointF arrays allocated by getpoints in aggdraw.cxx.

While doing some testing on a local build made with address sanitizer, I got an error that the allocator and deallocator for points in draw_line didn't match. This looks true to me:

getpoints, returning memory allocated with new PointF[]:

aggdraw/aggdraw.cxx

Line 894 in 66af911

xy = new PointF[n+1];

draw_line, calling getpoints and then releasing the PointF using simple delete:

aggdraw/aggdraw.cxx

Line 1190 in 66af911

delete xy;

SUMMARY:

AddressSanitizer: alloc-dealloc-mismatch (/.../libtools_build_sanitizers_asan-ubsan-py.so+0xa06c0) in operator delete(void*, unsigned long)

==1481283==ERROR: AddressSanitizer: alloc-dealloc-mismatch (operator new [] vs operator delete) on 0x603000050f80
SCARINESS: 10 (alloc-dealloc-mismatch)
#0 0x7ff93c34e6c0 in operator delete(void*, unsigned long) (/.../libtools_build_sanitizers_asan-ubsan-py.so+0xa06c0)
#1 0x7ff92b4a7ba4 in draw_line(DrawObject*, _object*) /tmp/pip-wheel-2XAzmg/aggdraw/aggdraw.cxx:1190:16
#2 0x7ff93bfaa2ee in call_function /.../Python-2.7.14/Python/ceval.c:4357:13

0x603000050f80 is located 0 bytes inside of 24-byte region [0x603000050f80,0x603000050f98)
allocated by thread T0 here:
#0 0x7ff93c34d450 in operator new[](unsigned long) (/.../libtools_build_sanitizers_asan-ubsan-py.so+0x9f450)
#1 0x7ff92b4a4c9a in getpoints(_object*, int*) /tmp/pip-wheel-2XAzmg/aggdraw/aggdraw.cxx:894:24
#2 0x7ff92b4a7aff in draw_line(DrawObject*, _object*) /tmp/pip-wheel-2XAzmg/aggdraw/aggdraw.cxx:1183:31
#3 0x7ff93bfaa2ee in call_function /.../Python-2.7.14/Python/ceval.c:4357:13

Testing after this change, address sanitizer seems happy.
Thanks, maintainers!

[djhoese]

I was hoping to merge #50 by @dov this week and finally make a new release of aggdraw. It looks like the new version would have this issue too. Maybe I can merge #50 and then we can update this PR to fix these issues. Doing it the other way around (this first then that) may make us lose these changes.

@adenyes Great work. Thanks for the PR. Could you tell me what commands you run to generate these warnings so I might run them myself?

[djhoese]

@adenyes Any advice for reproducing these messages you were getting?

[adenyes]

@djhoese AddressSanitizer is included with clang and gcc these days. The simplest way to enable AddressSanitizer for this module is probably to add the appropriate compiler and linker flags to the Extension constructor parameters in setup.py:168
extra_compile_args=["-fsanitize=address"],
extra_link_args=["-fsanitize=address"]

I run it with an address-sanitizer instrumented python using a custom build chain. If you only want to instrument the module, there may be other steps - linking asan as a DSO, LD_PRELOAD the asan dso from clang's libs, wherever it is on your platform. I have not tried this, though.

The errors above were printed during a unit test which includes aggdraw and uses it to draw a line.

[djhoese]

Hm I'm having trouble getting this to work on my system. I add the flags to setup.py and get an error when trying to import aggdraw that tells me to do:

DYLD_INSERT_LIBRARIES=/opt/local/lib/libgcc/libasan.5.dylib python -c "import aggdraw"

I tried running our selftest.py and don't seem to be getting any errors/warnings. This is with gcc 8 installed from macports (OSX obviously). @adenyes Do you have a minimal aggdraw use case that causes asan to hit the "bad" code?

[adenyes]

You might not be getting the error because alloc_dealloc_mismatch is disabled by default on Darwin and Windows though I don't know the reasons behind that.
You can set a flag to turn it back on, as described here https://github.com/google/sanitizers/wiki/AddressSanitizerFlags#run-time-flags

The minimal case is to create a draw object and pen then draw a line. Your graphics self test should hit it. I can provide more specific help after Thursday. Thank you for working on this.

[djhoese]

Hm I'll have to switch to linux:

Details

$ DYLD_INSERT_LIBRARIES=/opt/local/lib/libgcc/libasan.5.dylib ASAN_OPTIONS="verbosity=1:alloc_dealloc_mismatch=true" python selftest.py
==66517==AddressSanitizer: libc interceptors initialized
|| `[0x200000000000, 0x7fffffffffff]` || HighMem    ||
|| `[0x140000000000, 0x1fffffffffff]` || HighShadow ||
|| `[0x120000000000, 0x13ffffffffff]` || ShadowGap  ||
|| `[0x100000000000, 0x11ffffffffff]` || LowShadow  ||
|| `[0x000000000000, 0x0fffffffffff]` || LowMem     ||
MemToShadow(shadow): 0x120000000000 0x123fffffffff 0x128000000000 0x13ffffffffff
redzone=16
max_redzone=2048
quarantine_size_mb=256M
thread_local_quarantine_size_kb=1024K
malloc_context_size=30
SHADOW_SCALE: 3
SHADOW_GRANULARITY: 8
SHADOW_OFFSET: 0x100000000000
==66517==Installed the sigaction for signal 11
==66517==Installed the sigaction for signal 10
==66517==Installed the sigaction for signal 8
==66517==T0: stack [0x7ffedfb8a000,0x7ffee0b8a000) size 0x1000000; local=0x7ffee0b8759c
==66517==AddressSanitizer Init done
=========================================================== test session starts ===========================================================
platform darwin -- Python 3.7.3, pytest-5.0.1, py-1.8.0, pluggy-0.12.0
rootdir: /Users/davidh/repos/git/aggdraw
collected 11 items                                                                                                                        

selftest.py ...........                                                                                                             [100%]

======================================================== 11 passed in 0.15 seconds ========================================================
==66517==T1 TSDDtor
==66517==T3 TSDDtor
==66517==T2 TSDDtor
==66517==T1 exited
==66517==T2 exited
==66517==T3 exited

[djhoese]

Darn, can't get it to work on CentOS 7 with miniconda installed gcc either:

Details

DYLD_INSERT_LIBRARIES=/data/users/davidh/miniconda3/envs/satpy/lib/libasan.so.5 ASAN_OPTIONS="verbosity=1:alloc_dealloc_mismatch=true" /data/users/davidh/miniconda3/envs/satpy/bin/python -c "import selftest; selftest.test_pen()"
==188783== Parsed ASAN_OPTIONS: verbosity=1:alloc_dealloc_mismatch=true
==188783== AddressSanitizer: failed to intercept 'memcmp'
==188783== AddressSanitizer: failed to intercept 'memmove'
==188783== AddressSanitizer: failed to intercept 'memset'
==188783== AddressSanitizer: failed to intercept 'memcpy'
==188783== AddressSanitizer: failed to intercept 'strcat'
==188783== AddressSanitizer: failed to intercept 'strchr'
==188783== AddressSanitizer: failed to intercept 'strcmp'
==188783== AddressSanitizer: failed to intercept 'strcpy'
==188783== AddressSanitizer: failed to intercept 'strlen'
==188783== AddressSanitizer: failed to intercept 'strncat'
==188783== AddressSanitizer: failed to intercept 'strncmp'
==188783== AddressSanitizer: failed to intercept 'strncpy'
==188783== AddressSanitizer: failed to intercept 'strcasecmp'
==188783== AddressSanitizer: failed to intercept 'strncasecmp'
==188783== AddressSanitizer: failed to intercept 'strdup'
==188783== AddressSanitizer: failed to intercept 'strnlen'
==188783== AddressSanitizer: failed to intercept 'index'
==188783== AddressSanitizer: failed to intercept 'atoi'
==188783== AddressSanitizer: failed to intercept 'atol'
==188783== AddressSanitizer: failed to intercept 'strtol'
==188783== AddressSanitizer: failed to intercept 'atoll'
==188783== AddressSanitizer: failed to intercept 'strtoll'
==188783== AddressSanitizer: failed to intercept 'mlock'
==188783== AddressSanitizer: failed to intercept 'munlock'
==188783== AddressSanitizer: failed to intercept 'mlockall'
==188783== AddressSanitizer: failed to intercept 'munlockall'
==188783== AddressSanitizer: failed to intercept 'longjmp'
==188783== AddressSanitizer: failed to intercept 'sigaction'
==188783== AddressSanitizer: failed to intercept 'signal'
==188783== AddressSanitizer: failed to intercept 'swapcontext'
==188783== AddressSanitizer: failed to intercept '_longjmp'
==188783== AddressSanitizer: failed to intercept 'siglongjmp'
==188783== AddressSanitizer: failed to intercept 'pthread_create'
==188783== AddressSanitizer: libc interceptors initialized
|| `[0x10007fff8000, 0x7fffffffffff]` || HighMem    ||
|| `[0x02008fff7000, 0x10007fff7fff]` || HighShadow ||
|| `[0x00008fff7000, 0x02008fff6fff]` || ShadowGap  ||
|| `[0x00007fff8000, 0x00008fff6fff]` || LowShadow  ||
|| `[0x000000000000, 0x00007fff7fff]` || LowMem     ||
MemToShadow(shadow): 0x00008fff7000 0x000091ff6dff 0x004091ff6e00 0x02008fff6fff
red_zone=16
malloc_context_size=30
SHADOW_SCALE: 3
SHADOW_GRANULARITY: 8
SHADOW_OFFSET: 7fff8000
==188783== Installed the sigaction for signal 11
==188783== T0: stack [0x7ffd7de15000,0x7ffd7e615000) size 0x800000; local=0x7ffd7e6108dc
==188783== AddressSanitizer Init done

[djhoese]

Ah ok, test_pen doesn't hit it. test_graphics does:

$ DYLD_INSERT_LIBRARIES=/data/users/davidh/miniconda3/envs/satpy/lib/libasan.so.5 /data/users/davidh/miniconda3/envs/satpy/bin/python -c "import selftest; selftest.test_graphics2()"
=================================================================
==190230== ERROR: AddressSanitizer: alloc-dealloc-mismatch (operator new [] vs operator delete) on 0x60040000dff0
    #0 0x7f09de15cb5a (/data/users/davidh/miniconda3/envs/satpy/lib/libasan.so.0.0.0+0x11b5a)
    #1 0x7f09e10f4d0e (/data/users/davidh/miniconda3/envs/satpy/lib/python3.7/site-packages/aggdraw.cpython-37m-x86_64-linux-gnu.so+0x18d0e)
    #2 0x7f09e949f6ef (/data/users/davidh/miniconda3/envs/satpy/bin/python3.7+0x1686ef)
0x60040000dff0 is located 0 bytes inside of 16-byte region [0x60040000dff0,0x60040000e000)
allocated by thread T0 here:
    #0 0x7f09de15ca0a (/data/users/davidh/miniconda3/envs/satpy/lib/libasan.so.0.0.0+0x11a0a)
    #1 0x7f09e10ec185 (/data/users/davidh/miniconda3/envs/satpy/lib/python3.7/site-packages/aggdraw.cpython-37m-x86_64-linux-gnu.so+0x10185)
    #2 0x7f09e10f47f7 (/data/users/davidh/miniconda3/envs/satpy/lib/python3.7/site-packages/aggdraw.cpython-37m-x86_64-linux-gnu.so+0x187f7)
    #3 0x7f09e949f6ef (/data/users/davidh/miniconda3/envs/satpy/bin/python3.7+0x1686ef)
==190230== HINT: if you don't care about these warnings you may set ASAN_OPTIONS=alloc_dealloc_mismatch=0
==190230== ABORTING

[djhoese]

Once I was able to get this working on linux I verified that taking in your modifications and running all of the tests (and analyzing what was changed), this looks good to me.

Thanks for running these checks and fixing everything.