GNOME Bugzilla – Bug 771612
dual page causes cache access beyond the allocated buffer
Last modified: 2016-10-12 05:41:05 UTC
Created attachment 335793 [details] [review] patch I think this piece of code causes access beyond allocated buffer in get_height_to_page: static void ev_view_size_request_continuous_dual_page (EvView *view, GtkRequisition *requisition) { gint n_pages; n_pages = ev_document_get_n_pages (view->document) + 1; n_pages will eventually hit ev_view_get_height_to_page which tries to access cache->height_to_page[page]. That array, however, has only n_pages+1 elements, so ranging from 0 to n_pages. The last element is the NULL sentinel. Accessing n_pages+1 is beyond what has been allocated. First, I check those bounds in ev_view_get_height_to_page. Then, I removed the +1. I haven't observed weird behaviour, but I don't really know whether removing the +1 causes havoc anywhere else. I couldn't trace the history of this line beyond 2009 which already included the +1 without a comment.
Created attachment 335794 [details] [review] patch
Comment on attachment 335794 [details] [review] patch >From 665592aaa19b0718b7800f9f16a2652b5701b370 Mon Sep 17 00:00:00 2001 >From: Tobias Mueller <muelli@cryptobitch.de> >Date: Sun, 18 Sep 2016 08:47:19 +0200 >Subject: [PATCH 2/2] libview: using n_pages, not n_pages+1 in > size_request_continuous_dual_page > >It is unclear to me why it increases the n_pages by one. >It will only lead to accessing the cache out of bounds, >because the cache is bound by 0..n_pages-1 and a final NULL sentinel. >Accessing n_pages+1 would lead to reaching beyond the final NULL. This is not accurate, there are two arrays in the cache, one for dual view and another one for non dual view. The dual view one is allocated as: cache->dual_height_to_page = g_new0 (gdouble, n_pages + 2); >Note that the code is now pretty equivalent to the other >size request function. Only the requisition->width calculation is different. >It may be worth it to refactor both function into one to shave off some >lines. >--- > libview/ev-view.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/libview/ev-view.c b/libview/ev-view.c >index 01f0b04..ae73d15 100644 >--- a/libview/ev-view.c >+++ b/libview/ev-view.c >@@ -3876,7 +3876,7 @@ ev_view_size_request_continuous_dual_page (EvView *view, > { > gint n_pages; > >- n_pages = ev_document_get_n_pages (view->document) + 1; >+ n_pages = ev_document_get_n_pages (view->document); > get_page_y_offset (view, n_pages, &requisition->height); So, I think the actual problem is that we are accessing height_to_page array in ev_view_get_height_to_page() unconditionally. We should probably move the access to the arrays inside the if: h = cache->height_to_page[page]; dh = cache->dual_height_to_page[page]; if (height) *height = (gint)(h * view->scale + 0.5); if (dual_height) *dual_height = (gint)(dh * view->scale + 0.5); That could be: if (height) *height = (gint)(cache->height_to_page[page] * view->scale + 0.5); if (dual_height) *dual_height = (gint)(cache->dual_height_to_page[page] * view->scale + 0.5); Note that ev_view_get_height_to_page() is called with null height on dual view, and null dual_height on single view. > switch (view->sizing_mode) { >-- >2.7.4 >
Created attachment 337174 [details] [review] patch Thanks for the comments! Now it makes a bit more sense to me :) The patch now only accesses the cache if it needs to. It is unclear to me, still, whether it should be possible to request both height and dual_height. I have thus left the original structure in place. One could probably get rid of the two local variables as you describe.
Comment on attachment 337174 [details] [review] patch Thanks!
Comment on attachment 337174 [details] [review] patch Pushed to both branches, thanks!