After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 771612 - dual page causes cache access beyond the allocated buffer
dual page causes cache access beyond the allocated buffer
Status: RESOLVED FIXED
Product: evince
Classification: Core
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: Evince Maintainers
Evince Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-09-18 07:12 UTC by Tobias Mueller
Modified: 2016-10-12 05:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (6.62 KB, patch)
2016-09-18 07:12 UTC, Tobias Mueller
none Details | Review
patch (1.22 KB, patch)
2016-09-18 07:12 UTC, Tobias Mueller
none Details | Review
patch (1.29 KB, patch)
2016-10-07 14:14 UTC, Tobias Mueller
committed Details | Review

Description Tobias Mueller 2016-09-18 07:12:14 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.
Comment 1 Tobias Mueller 2016-09-18 07:12:37 UTC
Created attachment 335794 [details] [review]
patch
Comment 2 Carlos Garcia Campos 2016-09-20 16:53:18 UTC
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
>
Comment 3 Tobias Mueller 2016-10-07 14:14:58 UTC
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 4 Carlos Garcia Campos 2016-10-10 17:37:34 UTC
Comment on attachment 337174 [details] [review]
patch

Thanks!
Comment 5 Carlos Garcia Campos 2016-10-12 05:40:54 UTC
Comment on attachment 337174 [details] [review]
patch

Pushed to both branches, thanks!