Also, it would be nice if you could provide a way for us to replicate the issue, so that we can understand the specific condition causing it.
On 02/22, Lauchlin Wilkinson wrote:
Hi,recently I've come across a bug where wget is segfaulting when it reaches a condition in process.c line 1167. Namely it seems that somehow padding is being being set to a negative value which is then being used in memset and thus causing the segfault. It is a bit tricky to reproduce the problem as I can only seem to trigger the crash when wget is getting called via ssh from a shell script as part of a packer.io provisioner step. I'm guessing it has something to do with the way the ssh session is setting the pty options to do with width and height of the terminal. Even if the settings are odd, I'm thinking that wget still should be able to handle the situation gracefully and not segfault. Would it be a good idea at line 1167 of progress.c to add some validation to ensure padding is never set to a negative value before memset() is called? wget version and a dump from valgrind attached. - Lauchlin ########################################################## wget --version GNU Wget 1.17.1 built on linux-gnu. +digest -gpgme +https +ipv6 +iri +large-file -metalink +nls +ntlm +opie +psl +ssl/openssl Wgetrc: /etc/wgetrc (system) Locale: /usr/share/locale Compile: gcc -DHAVE_CONFIG_H -DSYSTEM_WGETRC="/etc/wgetrc" -DLOCALEDIR="/usr/share/locale" -I. -I../lib -I../lib -DHAVE_LIBSSL -DNDEBUG -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic Link: gcc -DHAVE_LIBSSL -DNDEBUG -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -luuid -lssl -lcrypto -lz -lpsl -lidn ftp-opie.o openssl.o http-ntlm.o ../lib/libgnu.a Copyright (C) 2015 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later <http://www.gnu.org/licenses/gpl.html>. This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Originally written by Hrvoje Niksic <[email protected]>. Please send bug reports and questions to <[email protected]>. ########################################################## ==1528== Memcheck, a memory error detector ==1528== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al. ==1528== Using Valgrind-3.10.0 and LibVEX; rerun with -h for copyright info ==1528== Command: wget -c -O index.html https://www.property.com.au/ ==1528== --2016-02-22 05:30:52-- https://www.property.com.au/ Resolving www.property.com.au (www.property.com.au)... 104.114.168.114 Connecting to www.property.com.au (www.property.com.au)|104.114.168.114|:443... connected. HTTP request sent, awaiting response... 301 Moved Permanently Location: /buy [following] --2016-02-22 05:30:54-- https://www.property.com.au/buy Reusing existing connection to www.property.com.au:443. HTTP request sent, awaiting response... 200 OK Length: unspecified [text/html] Saving to: ‘index.html’ index.html 0 --.-KB/s ==1528== Invalid write of size 8 ==1528== at 0x4C2EE57: memset (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==1528== by 0x42627A: memset (string3.h:84) ==1528== by 0x42627A: create_image (progress.c:1167) ==1528== by 0x42662A: bar_finish (progress.c:673) ==1528== by 0x429498: fd_read_body (retr.c:429) ==1528== by 0x419D75: read_response_body (http.c:1682) ==1528== by 0x41F778: gethttp (http.c:3753) ==1528== by 0x41FC08: http_loop (http.c:3971) ==1528== by 0x42A054: retrieve_url (retr.c:817) ==1528== by 0x406F2B: main (main.c:1868) ==1528== Address 0x10278090 is 0 bytes after a block of size 144 alloc'd ==1528== at 0x4C28BCD: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==1528== by 0x43ADC8: xmalloc (xmalloc.c:41) ==1528== by 0x4267DD: bar_create (progress.c:598) ==1528== by 0x429319: fd_read_body (retr.c:274) ==1528== by 0x419D75: read_response_body (http.c:1682) ==1528== by 0x41F778: gethttp (http.c:3753) ==1528== by 0x41FC08: http_loop (http.c:3971) ==1528== by 0x42A054: retrieve_url (retr.c:817) ==1528== by 0x406F2B: main (main.c:1868) ==1528== ==1528== ==1528== Process terminating with default action of signal 11 (SIGSEGV) ==1528== Bad permissions for mapped region at address 0x10553000 ==1528== at 0x4C2EE57: memset (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==1528== by 0x42627A: memset (string3.h:84) ==1528== by 0x42627A: create_image (progress.c:1167) ==1528== by 0x42662A: bar_finish (progress.c:673) ==1528== by 0x429498: fd_read_body (retr.c:429) ==1528== by 0x419D75: read_response_body (http.c:1682) ==1528== by 0x41F778: gethttp (http.c:3753) ==1528== by 0x41FC08: http_loop (http.c:3971) ==1528== by 0x42A054: retrieve_url (retr.c:817) ==1528== by 0x406F2B: main (main.c:1868) ==1528== Invalid read of size 8 ==1528== at 0x5D69209: __gconv_release_step (gconv_db.c:211) ==1528== by 0x5D6A0E3: __gconv_close_transform (gconv_db.c:784) ==1528== by 0x5DF22D6: _nl_cleanup_ctype (wcsmbsload.c:267) ==1528== by 0x5EAA112: _nl_archive_subfreeres (in /lib64/libc-2.17.so) ==1528== by 0x5EA9E2A: free_mem (in /lib64/libc-2.17.so) ==1528== by 0x5EAA511: __libc_freeres (in /lib64/libc-2.17.so) ==1528== by 0x4A24684: _vgnU_freeres (in /usr/lib64/valgrind/vgpreload_core-amd64-linux.so) ==1528== by 0x10278030: ??? ==1528== by 0x42627A: memset (string3.h:84) ==1528== by 0x42627A: create_image (progress.c:1167) ==1528== by 0x42662A: bar_finish (progress.c:673) ==1528== by 0x429498: fd_read_body (retr.c:429) ==1528== by 0x419D75: read_response_body (http.c:1682) ==1528== Address 0x2d2d2d2d2d2d2cb8 is not stack'd, malloc'd or (recently) free'd ==1528== ==1528== ==1528== Process terminating with default action of signal 11 (SIGSEGV) ==1528== General Protection Fault ==1528== at 0x5D69209: __gconv_release_step (gconv_db.c:211) ==1528== by 0x5D6A0E3: __gconv_close_transform (gconv_db.c:784) ==1528== by 0x5DF22D6: _nl_cleanup_ctype (wcsmbsload.c:267) ==1528== by 0x5EAA112: _nl_archive_subfreeres (in /lib64/libc-2.17.so) ==1528== by 0x5EA9E2A: free_mem (in /lib64/libc-2.17.so) ==1528== by 0x5EAA511: __libc_freeres (in /lib64/libc-2.17.so) ==1528== by 0x4A24684: _vgnU_freeres (in /usr/lib64/valgrind/vgpreload_core-amd64-linux.so) ==1528== by 0x10278030: ??? ==1528== by 0x42627A: memset (string3.h:84) ==1528== by 0x42627A: create_image (progress.c:1167) ==1528== by 0x42662A: bar_finish (progress.c:673) ==1528== by 0x429498: fd_read_body (retr.c:429) ==1528== by 0x419D75: read_response_body (http.c:1682) ==1528== ==1528== HEAP SUMMARY: ==1528== in use at exit: 923,752 bytes in 19,829 blocks ==1528== total heap usage: 38,638 allocs, 18,809 frees, 2,684,605 bytes allocated ==1528== ==1528== 208 bytes in 1 blocks are definitely lost in loss record 819 of 960 ==1528== at 0x4C28BCD: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==1528== by 0x5D7140E: __gconv_lookup_cache (gconv_cache.c:371) ==1528== by 0x5D69E29: __gconv_find_transform (gconv_db.c:721) ==1528== by 0x5DF2466: __wcsmbs_getfct (wcsmbsload.c:92) ==1528== by 0x5DF2466: __wcsmbs_load_conv (wcsmbsload.c:187) ==1528== by 0x5DE855C: get_gconv_fcts (wcsmbsload.h:75) ==1528== by 0x5DE855C: mbrtowc (mbrtowc.c:69) ==1528== by 0x5D7F4EB: mbtowc (mbtowc.c:64) ==1528== by 0x425307: count_cols (progress.c:804) ==1528== by 0x425F1B: create_image (progress.c:908) ==1528== by 0x4267FB: bar_create (progress.c:602) ==1528== by 0x429319: fd_read_body (retr.c:274) ==1528== by 0x419D75: read_response_body (http.c:1682) ==1528== by 0x41F778: gethttp (http.c:3753) ==1528== ==1528== 208 bytes in 1 blocks are definitely lost in loss record 820 of 960 ==1528== at 0x4C28BCD: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==1528== by 0x5D7140E: __gconv_lookup_cache (gconv_cache.c:371) ==1528== by 0x5D69E29: __gconv_find_transform (gconv_db.c:721) ==1528== by 0x5DF250D: __wcsmbs_getfct (wcsmbsload.c:92) ==1528== by 0x5DF250D: __wcsmbs_load_conv (wcsmbsload.c:190) ==1528== by 0x5DE855C: get_gconv_fcts (wcsmbsload.h:75) ==1528== by 0x5DE855C: mbrtowc (mbrtowc.c:69) ==1528== by 0x5D7F4EB: mbtowc (mbtowc.c:64) ==1528== by 0x425307: count_cols (progress.c:804) ==1528== by 0x425F1B: create_image (progress.c:908) ==1528== by 0x4267FB: bar_create (progress.c:602) ==1528== by 0x429319: fd_read_body (retr.c:274) ==1528== by 0x419D75: read_response_body (http.c:1682) ==1528== by 0x41F778: gethttp (http.c:3753) ==1528== ==1528== LEAK SUMMARY: ==1528== definitely lost: 416 bytes in 2 blocks ==1528== indirectly lost: 0 bytes in 0 blocks ==1528== possibly lost: 0 bytes in 0 blocks ==1528== still reachable: 923,336 bytes in 19,827 blocks ==1528== suppressed: 0 bytes in 0 blocks ==1528== Reachable blocks (those to which a pointer was found) are not shown. ==1528== To see them, rerun with: --leak-check=full --show-leak-kinds=all ==1528== ==1528== For counts of detected and suppressed errors, rerun with: -v ==1528== ERROR SUMMARY: 374197 errors from 4 contexts (suppressed: 1 from 1)
-- Thanking You, Darshit Shah
From 2f5e1ad62b833361c19b7e389718769ab01ef981 Mon Sep 17 00:00:00 2001 From: Darshit Shah <[email protected]> Date: Mon, 22 Feb 2016 15:08:15 +0100 Subject: [PATCH] Sanitize value sent to memset to prevent SEGFAULT --- src/progress.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/progress.c b/src/progress.c index 93f6246..8a5df21 100644 --- a/src/progress.c +++ b/src/progress.c @@ -1164,6 +1164,8 @@ create_image (struct bar_progress *bp, double dl_total_time, bool done) } padding = bp->width - count_cols (bp->buffer); + assert (padding > 0 && "Padding length became non-positive!"); + padding = padding > 0 ? padding : 0; memset (p, ' ', padding); p += padding; *p = '\0'; @@ -1174,6 +1176,9 @@ create_image (struct bar_progress *bp, double dl_total_time, bool done) * from the release code since we do not want Wget to crash and burn when the * assertion fails. Instead Wget should continue downloading and display a * horrible and irritating progress bar that spams the screen with newlines. + * + * By default, all assertions are disabled in a Wget build and are enabled + * only with the --enable-assert configure option. */ assert (count_cols (bp->buffer) == bp->width); } -- 2.7.1
signature.asc
Description: PGP signature
