Segfault when downloading tiles

[yourealwaysbe]

Commit c4c4914 introduced a segfault for me. It happens in the map download call on line 1751 of vikmapslayer.c:

      DownloadResult_t dr = vik_map_source_download( MAPS_LAYER_NTH_TYPE(mdi->maptype), &(mdi->mapcoord), mdi->filename_buf, handle);

The complete backtrace is included below. I had a poke around, but didn't see an obvious cause of the error. Perhaps there is a race condition somewhere?

[New Thread 0x7fffefc3d640 (LWP 55807)]
** viking [Warning]): No direction routing engines available
[New Thread 0x7fffef28d640 (LWP 55808)]

Thread 4 "pool-viking" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffef28d640 (LWP 55808)]
0x00007ffff72f0970 in ?? () from /usr/lib/libcurl.so.4
(gdb) backtrace
#0  0x00007ffff72f0970 in  () at /usr/lib/libcurl.so.4
#1  0x00007ffff72f3ab9 in curl_easy_setopt () at /usr/lib/libcurl.so.4
#2  0x00005555555d5fde in common_opts
    (curl=0x7fff00000001, uri=0x7fffe0b4ab90 "https://tile.thunderforest.com/cycle/16/32724/21801.png?apikey=<omited>", options=0x5555557e9378)
    at curl_download.c:126
#3  0x00005555555d6347 in curl_download_uri
    (uri=0x7fffe0b4ab90 "https://tile.thunderforest.com/cycle/16/32724/21801.png?apikey=<omited>", f=f@entry=0x7fffe0b4b820, options=0x5555557e9378, 
    options@entry=0x64bf52b2ff093a00, cdo=cdo@entry=0x7fffef28c780, handle=handle@entry=0x7fff00000001) at curl_download.c:160
#4  0x00005555555d6728 in curl_download_get_url
    (hostname=hostname@entry=0x0, uri=uri@entry=0x7fffe0b4ab90 "https://tile.thunderfo
rest.com/cycle/16/32724/21801.png?apikey=<omited>", f=f@entry=0x7fffe0b4b820, options=0x64bf52b2ff093a00, options@entry=0x5555557e9378, ftp=ftp@entry=0, cdo=cdo@entry=0x7fffef28c780, handle=0x7fff00000001) at curl_download.c:260
#5  0x0000555555608618 in download
    (hostname=hostname@entry=0x0, uri=uri@entry=0x7fffe0b4ab90 "https://tile.thunderforest.com/cycle/16/32724/21801.png?apikey=<omited>", fn=fn@entry=0x55555662be20 "/home/matt/.viking-maps/t17s1z0/32724/21801", options=0x5555557e9378, ftp=ftp@entry=0, handle=handle@entry=0x7fff00000001) at download.c:385
#6  0x00005555556090db in a_http_download_get_url
    (hostname=hostname@entry=0x0, uri=uri@entry=0x7fffe0b4ab90 "https://tile.thunderforest.com/cycle/16/32724/21801.png?apikey=<omited>", fn=fn@entr
y=0x55555662be20 "/home/matt/.viking-maps/t17s1z0/32724/21801", opt=<optimized out>, handle=handle@entry=0x7fff00000001) at download.c:456
#7  0x0000555555691e4f in _download (self=0x5555557e9460, src=0x555555bdba94, dest_fn=0x55555662be20 "/home/matt/.viking-maps/t17s1z0/32724/21801", handle=0x7fff00000001)
    at vikmapsourcedefault.c:546
#8  0x000055555565fa8c in map_download_thread (mdi=mdi@entry=0x555555bdba70, threaddata=threaddata@entry=0x55555662bdd0) at vikmapslayer.c:1751
#9  0x0000555555657f9c in thread_helper (args=0x55555662bdd0, user_data=<optimized out>) at background.c:172
#10 0x00007ffff7ed9be7 in  () at /usr/lib/libglib-2.0.so.0
#11 0x00007ffff7ed6d31 in  () at /usr/lib/libglib-2.0.so.0
#12 0x00007ffff6d9b3e9 in start_thread () at /usr/lib/libpthread.so.0
#13 0x00007ffff6cc7293 in clone () at /usr/lib/libc.so.6
(gdb) Quit

[rnorris]

If you can recreate it, is it possible to do an:

info threads

And maybe do a backtrace in each thread please.

[rnorris]

Needless to say but I've never had any issues.

One thing I don't understand in your backtrace is the parameters in "#4"

options=0x64bf52b2ff093a00, options@entry=0x5555557e9378

Options is repeated twice. How is this?

[yourealwaysbe]

Here's a log of info threads and thread backtraces.

For options and options@entry i think one is the current value of options, the other the value when the function was called.

[rnorris]

Everyday is a school day! (Learns about @entry output in gdb).

I wouldn't have thought that the options pointer should change, which possibly indicates some memory issue.

ATM I can't determine much wrong with commit c4c4914

However I've noticed something - attached a patch file named ".txt" format just to please github's attachment mechanism - in that the options is not quite thread robust.
Previously the option values for a Map were fully set on initialization and thus wouldn't change.
With the introduction of location dependent custom http headers - commit b2d9e41 - the option changes dependent on the call and between multiple calling threads could get unexpected values.

I don't expect this to fix your issue, but it's worth a try.

0001-Improve-thread-safety-of-DownloadFileOptions-usage.txt

[yourealwaysbe]

Thanks for this. I'll have a play around and see if i can see what might be causing it.

I tried the patch -- it didn't fix it unfortunately. Thought i'm not using any custom HTTP headers on my maps, so you're right it's not too surprising.

[yourealwaysbe]

The gdb log has changed though -- the error now happens at the end of the function:

Thread 4 "pool-viking" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffef3d2640 (LWP 18513)]
0x00007ffff72bab44 in curl_easy_cleanup () from /usr/lib/libcurl.so.4
(gdb) backtrace
#0  0x00007ffff72bab44 in curl_easy_cleanup () at /usr/lib/libcurl.so.4
#1  0x000055555565fd41 in map_download_thread
    (mdi=mdi@entry=0x555555d441f0, threaddata=threaddata@entry=0x555555daf340)
    at vikmapslayer.c:1793
#2  0x0000555555657fec in thread_helper
    (args=0x555555daf340, user_data=<optimized out>) at background.c:172
#3  0x00007ffff7ed8d77 in  () at /usr/lib/libglib-2.0.so.0
#4  0x00007ffff7ed5ec1 in  () at /usr/lib/libglib-2.0.so.0
#5  0x00007ffff6d9a3e9 in start_thread () at /usr/lib/libpthread.so.0
#6  0x00007ffff6cc6293 in clone () at /usr/lib/libc.so.6

[yourealwaysbe]

I have to head out now -- but here's what i found.

The map_download_thread function in vikmapslayer.c is reaching the end with handle = 1 when it segfaults. Normally handle is a 10 digit number. It then calls the clean up code, which passes the handle of 1 to curl_easy_cleanup, which crashes.

[yourealwaysbe]

Ok, i think i see what's changing the handle to 1. In map_download_thread in vikmapslayer.c there is

  gboolean needed[mdi->xf-mdi->x0][mdi->yf-mdi->y0];

  for ( x = mdi->x0; x <= mdi->xf; x++ ) {
    mcoord.x = x;
    for ( y = mdi->y0; y <= mdi->yf; y++ ) {

This should either be a +1 on the dimensions of the needed array, or a < instead of <= in the for loops.

The error "seems" to go away with the thread-safe patch and +1 to the needed array dimensions.

[rnorris]

Yes well done!

Needs +1 added to the array dimensions.

It may be 'luck' of the draw - depending on if the out of array bounds is actually written to, and then depending on the compilation what memory the out of bounds is and what the effect of unintentionally writing to it - crashing vs data corruption vs no observable change. Such that I never noticed before.