bugGNU Wget - Bugs: bug #54126, Wget keeps crashing in Windows...

 
 

bug #54126: Wget keeps crashing in Windows sometimes when the filename is large enough to scroll it

Submitted by:  Vyacheslav <vyacheslav>
Submitted on:  Sat 16 Jun 2018 10:43:59 PM UTC  
 
Category:  Crash/Freeze/Infloop Severity:  3 - Normal
Priority:  5 - Normal Status:  None
Privacy:  Public Assigned to:  Darshit Shah <darnir>
Originator Name:  Open/Closed:  Open
Release:  1.19.5 Operating System:  Microsoft Windows
Reproducibility:  Intermittent Fixed Release:  None
Planned Release:  1.21 Regression:  None
Work Required:  None Patch Included:  Yes

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Fri 20 Dec 2019 10:24:29 AM UTC, comment #9: 
Vyacheslav <vyacheslav>
Fri 20 Dec 2019 09:05:59 AM UTC, comment #8: 

Sorry, i was wrong (
when progress fuzzed with defined USE_NLS_PROGRESS_BAR crash happens if filename contain ctrl chars after invalid mbc.
(it may be reprodused on released build:
wget $url -O $filename
where $filename contains: \255 many \2 \255 (who knows, maybe this could happen)
restrict_files_ctrl doesnt affect on -O option)
the reason we get negative padding is because count_cols () and cols_to_bytes() count cols in two different ways:
(count_cols() after invalid mbc simply add the rest bytes in string[it may cause problem wo ctrl char], cols_to_bytes() continue counting & skips ctrl chars [mb_width()=0])
if we make count_cols() like cols_to_bytes() assert will be passed, but we still can get buffer overflow

padding=bp->width-count_cols (bp->buffer);
memset (p, ' ', padding);

padding will be too long (in buffer we already have space occuped by ctrl chars).
it may be fixed by copy filename and replacing non printable chars in filename with hexdgt code, or just removing them.
(ideally, like you say scrolling has to be rewritten, but ctrl chars must be removed anyway)

Vyacheslav <vyacheslav>
Mon 16 Dec 2019 11:30:43 AM UTC, comment #7: 

Sorry, I am not sure what your point is.

You say "when USE_NLS_PROGRESS_BAR defined all works fine".

But no, not at all. All those changes to prevent different types of buffer and integer overflow have only been tested with USE_NLS_PROGRESS_BAR defined.

I now applied your progress bar patch and pushed to branch 'tmp-progress-fuzzer' on Gitlab for testing. But the scrolling code still is commented, as it still creates buffer overflows when USE_NLS_PROGRESS_BAR is defined. Not fuzzed with USE_NLS_PROGRESS_BAR undefined yet.

Tim Ruehsen <rockdaboot>
Project Administrator
Sun 15 Dec 2019 08:19:51 PM UTC, comment #6: 

комментарий №5:

> I fixed a bunch of issues found by fuzzing in the progress (bar +
> dot) code. The 'filename scrolling' part of the progress bar needed to be disabled and likely has to be rewritten.


that build must success, later i will try build, BUT ???

Fix buffer overflows in progress 'bar' code
#define BUF_LEN (bp->width * 2 + 100)
must fix overflow without disabling "filename scrolling", and of course, disabled scrolling will fix problem, but why ignored wrong cols_to_bytes()??? With patched cols_to_bytes no needed rewrite 'filename scroling', it works as expected. With all respect it is not solution (when USE_NLS_PROGRESS_BAR defined all works fine), May be there is an other reason for it?

let's: strlen(bp->f_download)=52, offset_bytes=49, bp->width=139, MAX_FILENAME_COLS = 34
at line 988:

bytes_in_filename = cols_to_bytes (bp->f_download + offset_bytes, MAX_FILENAME_COLS - padding, *cols_ret)

bytes_in_filename will be 34 (MAX_FILENAME_COLS, padding is 0 [padding can not be negative]), but actually bytes_in_filename is 3
Further, at line 992:

memcpy (p, bp->f_download + offset_bytes, bytes_in_filename);

we are "happy" to copy the rest of the string, including termination '\0' and a few more bytes after the end of the string, this is not fatal yet  (just the reason for repeating the last letter), but the beginning of this.
bad thing happens at line 1161:

  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';

padding will be too long (136), because in bp->buffer we have '\0' at offset 3, therefore memset causes buffer overflow (width = 139, we are almost at the end position, buffer = 139 + 100),
an overflow of 32 bytes (see bar_log_XXX.log in file #48068), and will be blessed this process by the memory allocation manager and thread context switcher, and let them be lucky in this (often they got lucky). For what purposes wget and msvcrt allocate memory in this block ??? in most cases for the file * output structure and output buffer. Mostly crashes occur when  RtlFreeHeap is called, after free or fclose.

thanks fflush is called every time after fwrite (retr.c) or otherwise time to time we can got broken downloaded file.
see attached simple_write_test file in (file #48068) (inexplicable "\32\32\32\32\32\0" or "\32\32\32\32\32"...,  suspect part of simple_write_test stream buffer was owerwriten, also an interesting address in the crash report 0x61616161 "aaaa"). A lot of interesting things can happen as a result of a buffer overflow...

Vyacheslav <vyacheslav>
Fri 13 Dec 2019 02:41:39 PM UTC, comment #5: 

From the mailing list:

I fixed a bunch of issues found by fuzzing in the progress (bar +
dot) code. The 'filename scrolling' part of the progress bar needed to be disabled and likely has to be rewritten.

The changes are in branch 'tmp-progress-fuzzer' in
https://gitlab.com/gnuwget/wget.git.

Is it possible that you build wget from that branch and test it ?
If you report success, the changes can be pushed to master.

Tim Ruehsen <rockdaboot>
Project Administrator
Wed 04 Dec 2019 12:29:24 PM UTC, comment #4: 

Given a fast enough internet connection, it may be necessary to use a bigger file for the script to succeed consistently.
In that case, I suggest replacing the URL in the script for this:
http://www.ovh.net/files/1Gio.dat

I've recorded a demonstration to show how consistent this crash can be: https://streamable.com/gas8q

FailedShack <failedshack>
Tue 03 Dec 2019 09:03:11 PM UTC, comment #3: 

I was running into this bug myself and spent a bit of time wondering why wget kept crashing.
So I played around with it until I narrowed down the issue to the progress bar, just to then find this bug report.

I can reproduce this issue every time the filename has to scroll and the download takes long enough for the text to go out of bounds at least once,
which happens to be a really common occurrence. Hope this patch is merged soon as it is pretty annoying.

This batch file should reproduce the crash every time:

@echo off
set file=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
:test
wget http://www.ovh.net/files/100Mio.dat -O %file%
if %errorlevel% == 0 (
    rem Fluke. Try again.
    goto test
) else (
    echo Wget failed with status: 0x%=exitcode%
)
del %file%
pause >nul

FailedShack <failedshack>
Fri 05 Jul 2019 10:18:14 PM UTC, comment #2: 

comment #1:

> Seems like this only occurs under Windows, which makes it slightly problematic when reproducing.


Yes, Linux uses USE_NLS_PROGRESS_BAR
in the function cols_to_byte() we see

while (p_cols <cols && _mbi_avail_ _(iter) ) {
.....
if (p_cols> cols)
{
p_cols - = mb_width (mbc);
break;
}
.....
}

when USE_NLS_PROGRESS_BAR is undefined
cols_to_byte returns cols regardless length of the remainder of the string  (mbi_avail (iter))
This is the reason for repeating the last letter.
I think crash and freezes hapends in create_image ()

....
offset_bytes   =    cols_to_bytes(bp->f_download,
                                  offset_cols,
                                  cols_ret);
bytes_in_filename = cols_to_bytes(bp-> f_download + offset_bytes,
                                  MAX_FILENAME_COLS - padding,
                                  cols_ret);
//here we can read after the end of the string
memcpy (p, bp-> f_download + offset_bytes, bytes_in_filename);
p + = bytes_in_filename;
padding = MAX_FILENAME_COLS - (padding + * cols_ret);
// padding can be negative_
//            (size_t) -3 ???
memset (p, '', padding + 1);
p + = padding + 1;

debug build is free from crashes.
anyway after applying

-  *ncols = cols;
-  return cols;
+  int len = strlen(mbs);
+  int ret = (len < cols ? len : cols);
+  *ncols = ret;
+  return ret;

crashes disapieres
but unsure about japanese, korean, chinese

Anonymous
Mon 12 Nov 2018 10:25:05 PM UTC, comment #1: 

Thanks for the report! I'll take a look at the issue and see if I can spot any problems. Seems like this only occurs under Windows, which makes it slightly problematic when reproducing.

Darshit Shah <darnir>
Project AdministratorIn charge of this item.
Sat 16 Jun 2018 10:43:59 PM UTC, original submission:  

the last letter keeps getting repeated, while the rest of the progress bar stops updating.

I'm not very familiar with the Debuggers under Wine, so I'm just
added some code to progress.c, which writes to the logfile information about the function calls
and the values of the variables in this functions:
wne-console ./wget-deb.exe http://xxx.yyy.zz -O 01234567890123456789abcdef.iso
..........

>-bar_draw(bp->f_download=01234567890123456789abcdef.iso)

    ........
    >-create_image(fName=01234567890123456789abcdef.iso)
        ........
        ........
        //offset_bytes = cols_to_bytes (bp->f_download, offset_cols, cols_ret);
        >-cols_to_bytes(mbs=01234567890123456789abcdef.iso, cols=13)
              strlen(mbs)=30
              return 13
        <-cols_to_bytes (mbs=01234567890123456789abcdef.iso)
        offset_bytes = 13
        >-cols_to_bytes (mbs=3456789abcdef.iso, cols=19)
              strlen(mbs)=17
              return 19
        <-cols_to_bytes (mbs=3456789abcdef.iso)
        bytes_in_filename = 19 !!!!! BUT it is not!
        cols_ret = 19
        //memcpy (p, bp->f_download + offset_bytes, bytes_in_filename);
        #buffer/start=offset(13)/#         = #3456789abcdef.iso#
        padding = 0 => MAX_FILENAME_COLS - (padding + *cols_ret) [ 19 - (0 + 19)]   !!!!!! BUT must be real cols_bytes for padding!!
        space set at *p !!!!! always be at same place!
        #buffer/start=offset/padded/#      = #3456789abcdef.iso# !!!! must be #3456789abcdef.iso #!
        .......
        .......
    <-create_image(fName=01234567890123456789abcdef.iso)
    .........
<-bar_draw(bp->f_download=01234567890123456789abcdef.iso)
..............
And again and again.  cols_to_bytes() simply returns cols , regardless length of the string.
I think that sometimes memcpy goes beyond the boundaries of the memory belonging to the process,
depending on the length of the string and the memory allocation for it.

some changes in cols_to_bytes() and freezes disappear, no crashes for now yet.

Vyacheslav <vyacheslav>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #48068:  wget_bar_log_crush_reports.tar.xz added by vyacheslav (16KiB - application/octet-stream - bar log with value of variables in it. crash reports [wget compiled with -g option])
file #47983:  wget.exe.9896.dmp added by failedshack (678KiB - application/octet-stream - Crash dump generated on Windows)
file #44378:  wget-patch.tar.xz added by vyacheslav (4KiB - application/x-xz - all patches that used in my build)
file #44376:  wget-version.txt added by vyacheslav (2KiB - text/plain)
file #44377:  progress-win32.patch added by vyacheslav (384B - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rockdaboot (Posted a comment)
  • -email is unavailable- added by failedshack (Posted a comment)
  • -email is unavailable- added by darnir (Posted a comment)
  • -email is unavailable- added by vyacheslav (Submitted the item)
  • -email is unavailable- added by vyacheslav
  •  

     

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-12-15 vyacheslav Attached File- => Added wget_bar_log_crush_reports.tar.xz, #48068
    2019-12-03 failedshack Attached File- => Added wget.exe.9896.dmp, #47983
    2018-11-12 darnir Assigned toNone => darnir
        Planned ReleaseNone => 1.21
    2018-06-16 vyacheslav Attached File- => Added wget-patch.tar.xz, #44378
    2018-06-16 vyacheslav Attached File- => Added wget-version.txt, #44376
        Attached File- => Added progress-win32.patch, #44377
        Carbon-Copy- => Added vyacheslav

    Back to the top


    Powered by Savane 3.5