Simple scan crashes when a PDF is saved

[Stef Walter]

SOLUTION
*******************************
This was one of the most prominent bugs in Simple Scan for a long time.
It has been fixed in Simple Scan 3.3.92
Upgrade to Simple Scan 3.3.92, older versions are still affected but will not be fixed.
*******************************

When saving a PDF memory corruption occurs and simple scan crashes in random code (for me in the deflate functionality). Checked this using clean bzr checkout. BTW, I would have patched this much earlier if simple-scan was version control system that I was familiar with (like git) :S

Can be verified with valgrind:

** WARNING **: scanner.vala:1204: Scan completed with 2250 lines, expected 2250 lines
==8804== Thread 1:
==8804== Invalid write of size 1
==8804== at 0x40FCFA: book_save_pdf (book.c:1826)
==8804== by 0x411F20: book_save (book.c:2533)
==8804== by 0x44372F: simple_scan_save_document (ui.c:1638)
==8804== by 0x447230: save_file_button_clicked_cb (ui.c:3002)
==8804== by 0x66AFD53: g_cclosure_marshal_VOID__VOID (gmarshal.c:85)
==8804== by 0x66ADF59: g_closure_invoke (gclosure.c:774)
==8804== by 0x66C7C40: signal_emit_unlocked_R (gsignal.c:3302)
==8804== by 0x66C6E51: g_signal_emit_valist (gsignal.c:3033)
==8804== by 0x66C7507: g_signal_emit_by_name (gsignal.c:3127)
==8804== by 0x4F14CBC: button_clicked (gtktoolbutton.c:881)
==8804== by 0x66AFD53: g_cclosure_marshal_VOID__VOID (gmarshal.c:85)
==8804== by 0x66ADF59: g_closure_invoke (gclosure.c:774)
==8804== Address 0x2102c5c8 is 0 bytes after a block of size 711,000 alloc'd
==8804== at 0x4A05BB4: calloc (vg_replace_malloc.c:467)
==8804== by 0x6947193: standard_calloc (gmem.c:104)
==8804== by 0x6947225: g_malloc0 (gmem.c:189)
==8804== by 0x69474E2: g_malloc0_n (gmem.c:385)
==8804== by 0x40F889: book_save_pdf (book.c:1674)
==8804== by 0x411F20: book_save (book.c:2533)
==8804== by 0x44372F: simple_scan_save_document (ui.c:1638)
==8804== by 0x447230: save_file_button_clicked_cb (ui.c:3002)
==8804== by 0x66AFD53: g_cclosure_marshal_VOID__VOID (gmarshal.c:85)
==8804== by 0x66ADF59: g_closure_invoke (gclosure.c:774)
==8804== by 0x66C7C40: signal_emit_unlocked_R (gsignal.c:3302)
==8804== by 0x66C6E51: g_signal_emit_valist (gsignal.c:3033)
==8804==
==8804== Invalid read of size 1
==8804== at 0x40FD0C: book_save_pdf (book.c:1827)
==8804== by 0x411F20: book_save (book.c:2533)
==8804== by 0x44372F: simple_scan_save_document (ui.c:1638)
==8804== by 0x447230: save_file_button_clicked_cb (ui.c:3002)
==8804== by 0x66AFD53: g_cclosure_marshal_VOID__VOID (gmarshal.c:85)
==8804== by 0x66ADF59: g_closure_invoke (gclosure.c:774)
==8804== by 0x66C7C40: signal_emit_unlocked_R (gsignal.c:3302)
==8804== by 0x66C6E51: g_signal_emit_valist (gsignal.c:3033)
==8804== by 0x66C7507: g_signal_emit_by_name (gsignal.c:3127)
==8804== by 0x4F14CBC: button_clicked (gtktoolbutton.c:881)
==8804== by 0x66AFD53: g_cclosure_marshal_VOID__VOID (gmarshal.c:85)
==8804== by 0x66ADF59: g_closure_invoke (gclosure.c:774)
==8804== Address 0x2102c5c8 is 0 bytes after a block of size 711,000 alloc'd
==8804== at 0x4A05BB4: calloc (vg_replace_malloc.c:467)
==8804== by 0x6947193: standard_calloc (gmem.c:104)
==8804== by 0x6947225: g_malloc0 (gmem.c:189)
==8804== by 0x69474E2: g_malloc0_n (gmem.c:385)
==8804== by 0x40F889: book_save_pdf (book.c:1674)
==8804== by 0x411F20: book_save (book.c:2533)
==8804== by 0x44372F: simple_scan_save_document (ui.c:1638)
==8804== by 0x447230: save_file_button_clicked_cb (ui.c:3002)
==8804== by 0x66AFD53: g_cclosure_marshal_VOID__VOID (gmarshal.c:85)
==8804== by 0x66ADF59: g_closure_invoke (gclosure.c:774)
==8804== by 0x66C7C40: signal_emit_unlocked_R (gsignal.c:3302)
==8804== by 0x66C6E51: g_signal_emit_valist (gsignal.c:3033)
==8804==

The problem is that due to a integer rounding error, one byte less is allocated in the image buffer than there should be. I don't understand the code completely, so this patch should be verified by the original author of the code. Attached.

[Michael Nagel (nailor)]

Awesome! I'll ping Robert about this, so he can double check.
I am quite optimistic that this might also fix Bug #664608 -- Then again, if it does, I should have bet some real money against Robert :)

About your git side blow: I have been thinking about this. The fragmentation of version control systems surely isn't really helping anyone, and it looks as if git was standing strong and gaining market share, unlike most (all?) alternatives, bazaar included. Then again, launchpad only supports bazaar right now, and I do not think giving up on launchpad is advisable for simple scan...

[Stef Walter (stefw)]

Wonderful. Looking forward to getting this fixed. Simple scan is just wonderful, modulo this bug :)

Yeah, the problem is that it seems to me harder to get contributors from the wider community when using unfamiliar tools. Whether the perceptions of how different or difficult bzr is, is waranted or not...

In my case I wanted to fix this several months ago, but just moved on because of the bzr. Finally when I encountered this again, I installed bzr, and it was surprisingly easy to figure out the two commands I needed (branch and diff).

[Robert Ancell (robert-ancell)]

I've been playing around with this patch but I can't actually find any case where it can overwrite this buffer.

This is interesting however:
** WARNING **: scanner.vala:1204: Scan completed with 2250 lines, expected 2250 lines
(the log message is actually wrong and is showing the same variable there - fixed in trunk)

Could you perhaps throw some printfs in there to dump out what the size of the buffer was, what the other variables were and check on each write and log if it does overflow? I'm concerned if this isn't the cause we're just hiding another problem.

[Stef Walter (stefw)]

Sure thing. Will try to do that. But it'll probably take a while before i can reproduce this. Like I said on IRC it only happens with certain documents.

[Stef Walter (stefw)]

Added the attached printfs. Here's the output:

** WARNING **: scanner.vala:1204: Scan completed with 1584 lines, expected 2250 lines
allocated: data_length = 711000, height = 2250, width = 1264
shift_count: offset 711000 >= data_length 711000 (x = 1263, width = 1264)
simple-scan: malloc.c:2453: sYSMALLOc: Assertion `(old_top == (((mbinptr) (((char *) &((av)->bins[((1) - 1) * 2])) - __builtin_offsetof (struct malloc_chunk, fd)))) && old_size == 0) || ((unsigned long) (old_size) >= (unsigned long)((((__builtin_offsetof (struct malloc_chunk, fd_nextsize))+((2 * (sizeof(size_t))) - 1)) & ~((2 * (sizeof(size_t))) - 1))) && ((old_top)->size & 0x1) && ((unsigned long)old_end & pagemask) == 0)' failed.
Aborted (core dumped)

This is a buffer overrun by one byte. You can see clearly that all the other color depths (besides DeviceGray) add an extra byte to account for this overflow. Only the DeviceGray one does not, adding one to the data_length calculations for DeviceGray fixes the problem.

Please let me know if you want any other printfs or information in specific places.

[Robert Ancell (robert-ancell)]

Thanks Stef. Will look at those asap.

[Robert Ancell (robert-ancell)]

Thanks for the diagnosis Stef. Your log shows the overrun is when you write the last bits of the page - it then zeroed the next byte in preparation to write another bit. I've fixed this by only zeroing out this byte when about to write. I've also removed the +1 from the color page writes - they shouldn't be necessary.

http://bazaar.launchpad.net/~simple-scan-team/simple-scan/trunk/revision/567

[Robert Ancell (robert-ancell)]

Fixed in 3.3.92

[Stef Walter (stefw)]

Cool. Thanks. Just built and tested it.