sumatrapdfreader / sumatrapdf

SumatraPDF reader
http://www.sumatrapdfreader.org
GNU General Public License v3.0
13.63k stars 1.73k forks source link

Crashes if "Save Annotations" repeatedly #3704

Closed kerryland closed 1 year ago

kerryland commented 1 year ago

453086de08320c38b3178c7930cc910e6013e2a7 crashes:

No file corruption though, so that's excellent.

kjk commented 1 year ago

If in debugger, please post callstack.

I got the crash on emma file with:

SumatraPDF.exe!__report_gsfailure(unsigned __int64 stack_cookie) Line 220
    at D:\a\_work\1\s\src\vctools\crt\vcstartup\src\gs\gs_report.c(220)
SumatraPDF.exe!__GSHandlerCheck(_EXCEPTION_RECORD * ExceptionRecord, void * EstablisherFrame, _CONTEXT * ContextRecord, _DISPATCHER_CONTEXT * DispatcherContext) Line 93
    at D:\a\_work\1\s\src\vctools\crt\vcstartup\src\gs\amd64\gshandler.cpp(93)
ntdll.dll!00007ffa42b93dff()
ntdll.dll!00007ffa42b0e456()
ntdll.dll!00007ffa42b92dee()
SumatraPDF.exe!fz_strlcpy(char * dst, const char * src, unsigned __int64 siz) Line 140
    at C:\Users\kjk\src\sumatrapdf\mupdf\source\fitz\string.c(140)
SumatraPDF.exe!fz_vwarn(fz_context * ctx, const char * fmt, char * ap) Line 119
    at C:\Users\kjk\src\sumatrapdf\mupdf\source\fitz\error.c(119)
SumatraPDF.exe!fz_warn(fz_context * ctx, const char * fmt, ...) Line 128
    at C:\Users\kjk\src\sumatrapdf\mupdf\source\fitz\error.c(128)
SumatraPDF.exe!seek_file(fz_context * ctx, fz_stream * stm, __int64 offset, int whence) Line 139
    at C:\Users\kjk\src\sumatrapdf\mupdf\source\fitz\stream-open.c(139)
SumatraPDF.exe!fz_seek(fz_context * ctx, fz_stream * stm, __int64 offset, int whence) Line 184
    at C:\Users\kjk\src\sumatrapdf\mupdf\source\fitz\stream-read.c(184)
SumatraPDF.exe!ensure_initial_incremental_contents(fz_context * ctx, fz_stream * in, fz_output * out, __int64 len) Line 3393
    at C:\Users\kjk\src\sumatrapdf\mupdf\source\pdf\pdf-write.c(3393)
SumatraPDF.exe!do_pdf_save_document(fz_context * ctx, pdf_document * doc, pdf_write_state * opts, const pdf_write_options * in_opts) Line 3445
    at C:\Users\kjk\src\sumatrapdf\mupdf\source\pdf\pdf-write.c(3445)
SumatraPDF.exe!pdf_save_document(fz_context * ctx, pdf_document * doc, const char * filename, const pdf_write_options * in_opts) Line 3804
    at C:\Users\kjk\src\sumatrapdf\mupdf\source\pdf\pdf-write.c(3804)
SumatraPDF.exe!EngineMupdfSaveUpdated(EngineBase * engine, const char * path, std::function<void __cdecl(char const *)> showErrorFunc) Line 3333
    at C:\Users\kjk\src\sumatrapdf\src\EngineMupdf.cpp(3333)
SumatraPDF.exe!SaveAnnotationsToExistingFile(WindowTab * tab) Line 2306
    at C:\Users\kjk\src\sumatrapdf\src\SumatraPDF.cpp(2306)
SumatraPDF.exe!FrameOnCommand(MainWindow * win, HWND__ * hwnd, unsigned int msg, unsigned __int64 wp, __int64 lp) Line 4995
    at C:\Users\kjk\src\sumatrapdf\src\SumatraPDF.cpp(4995)
SumatraPDF.exe!WndProcSumatraFrame(HWND__ * hwnd, unsigned int msg, unsigned __int64 wp, __int64 lp) Line 5561
    at C:\Users\kjk\src\sumatrapdf\src\SumatraPDF.cpp(5561)

Also got log:

Saved annotations to 'C:\Users\kjk\!sumatra\bugs\bug3551-Emma - Jane Austen (1) - 3.pdf' in  22.19 ms
ReloadDocument: C:\Users\kjk\!sumatra\bugs\bug3551-Emma - Jane Austen (1) - 3.pdf, auto refresh: 0
CreateControllerForEngineOrFile: 'C:\Users\kjk\!sumatra\bugs\bug3551-Emma - Jane Austen (1) - 3.pdf', 268 pages
DisplayModel::BuildPagesInfo started
DisplayModel::BuildPagesInfo took 0.01 ms
expected object number
read error; treating as end of file
Exception thrown at 0x00007FF710BAE256 in SumatraPDF.exe: 0xC0000005: Access violation writing location 0x0000006A54B1023C.
Unhandled exception at 0x00007FF710DCE689 in SumatraPDF.exe: Stack cookie instrumentation code detected a stack-based buffer overrun.

Interestingly it worked fine several times and only the last save caused mupdf error read error; treating as end of file which suggests that the file from previous save wasn't fully written.

This could be a virus software still messing with the file.

Shouldn't cause mupdf to crash, though.

As a hacky work-around I could probably delay subsequent saves by a few seconds.

kjk commented 1 year ago

It definitely has something to do with how much time is between saves.

I did 10 saves with a second or two between them just fine.

But If I just hold Ctrl + Shift + S, it crashes immediately.

kjk commented 1 year ago

Also related to the size of the file: can't repro on small file from #3551.

kjk commented 1 year ago

No longer saving annotation if there are no annotation changes.

This puts a natural limit on how quickly we re-save (user has to add at least one annotation).

I tried to get it to crash by adding a annotations as quickly as I can and Ctrl + Shift + S and couldn't crash it.

GitHubRulesOK commented 1 year ago

Annots due to their additive nature should not be saved in a short time frame as they accumulate overheads ideally save should be delayed by minutes or hours to end of a stable session not seconds.

kerryland commented 1 year ago

@GitHubRulesOK

Annots due to their additive nature should not be saved in a short time frame as they accumulate overheads ideally save should be delayed by minutes or hours to end of a stable session not seconds.

If all you are doing is adding new annotations, rather than changing existing ones, it shouldn't make a difference, right?

Results in the same file as:

Right?

kjk commented 1 year ago

Semantically, yes. Physically: not.

I don't know the details but incremental saving is trying to only append new annotations to the end of the file (as opposed to as writing the whole PDF from scratch).

So doing 2 incremental saves will end up with a different file than saving those 2 annotations in one incremental save.

That being said users will do what they want to do. We have no power to mandate how often will they save the annotations.

GitHubRulesOK commented 1 year ago

@kerryland

1) add annotation of a few bytes & save = increase file by say 1-2 KB (or more in some cases, because the old related page data needs update)

2) add a replacement & delete former annotation & save = increase file by 3 or more KB because old annotations are not deleted just flagged as "no show" and both page areas affected need duplicate page index data.

3) add another annotation of a few bytes and now its say 5 KB extra, because all prior changes may need incremental inclusion as updated entries

The way to not bloat the file is in same session add and delete all you like and only those remaining get flushed to the file and index area is only appended once as required.

Edge and Acrobat Reader do a full re-write of file (in many cases) thus after annotation, the file can actually decrease in size.