2024-03-28 15:51 UTC

View Issue Details Jump to Notes ]
IDProjectCategoryView StatusLast Update
0002553NetSurf[All Projects] Generalpublic2017-10-16 22:33
ReporterAlastair Hughes 
Assigned ToVincent Sanders 
SeveritycrashReproducibilityalways 
StatusclosedResolutionopen 
Platformx86_64OSLinuxOS VersionArch
Product Version3.6 
Target VersionFixed in Version3.7 
Summary0002553: Unchecked *alloc() causes segfault in low-memory situations
DescriptionUnchecked *alloc() calls can cause segmentation faults in low-memory situations, such as on computers with very limited memory (eg 32mb) or when artificially restricting the amount of memory available. This is mainly a problem with the framebuffer frontend since a segmentation fault seems to leave the framebuffer in an unusable state.

This is present in (at least) the git version of netsurf and libnsfb; it looks like a larger audit would be necessary to find all the places where this occurs, however grepping for "alloc(" seems to find most of the locations to check.

Actually handling the failures seems to be a different story entirely, but avoiding crashing would be a nice start as it would at least allow the user to quit the program or (if there is sufficient memory) navigate to a different website.
Steps To Reproduceulimit -v 17000; netsurf

If this does not fail, try to travel to a website

(The ulimit -v value may need adjusting; glib aborts when *alloc() and similar fails, so testing should probably be with the framebuffer frontend).
Additional InformationI'm happy to go through and try to add some basic error-checking to the various *alloc() calls. For libnsfb error handling seems largely straightforward (functions returning false to indicate that they failed, right?) but whether or not the error is printed to stderr seems to vary between the various backends - is there some kind of policy here?
TagsNo tags attached.
Fixed in CI build #
Reported in CI build #
URL of problem page
Attached Files
  • patch file icon 0001-Always-check-return-value-for-alloc.patch (4,639 bytes) 2017-08-06 09:00 -
    From c7af6f4fc5ca1ba083974de2ef24a264ff92a654 Mon Sep 17 00:00:00 2001
    From: Alastair Hughes <hobbitalastair@gmail.com>
    Date: Sun, 6 Aug 2017 20:39:11 +1200
    Subject: [PATCH] Always check return value for *alloc()
    
    ---
     src/cursor.c       |  2 ++
     src/plot/generic.c |  4 ++++
     src/surface/ram.c  |  4 ++++
     src/surface/wld.c  | 15 +++++++++------
     src/surface/x.c    | 10 ++++++----
     5 files changed, 25 insertions(+), 10 deletions(-)
    
    diff --git a/src/cursor.c b/src/cursor.c
    index 87633dc..a18eba0 100644
    --- a/src/cursor.c
    +++ b/src/cursor.c
    @@ -100,6 +100,8 @@ bool nsfb_cursor_plot(nsfb_t *nsfb, struct nsfb_cursor_s *cursor)
         sav_size = cursor->sav_width * cursor->sav_height * sizeof(nsfb_colour_t);
         if (cursor->sav_size < sav_size) {
             cursor->sav = realloc(cursor->sav, sav_size);
    +        if (!cursor->sav)
    +            return false;
             cursor->sav_size = sav_size;
         }
     
    diff --git a/src/plot/generic.c b/src/plot/generic.c
    index 0c3d9e8..50bfe1a 100644
    --- a/src/plot/generic.c
    +++ b/src/plot/generic.c
    @@ -799,6 +799,8 @@ path(nsfb_t *nsfb, int pathc, nsfb_plot_pathop_t *pathop, nsfb_plot_pen_t *pen)
     
         /* allocate storage for the vertexes */
         curpt = pts = malloc(ptc * sizeof(nsfb_point_t));
    +    if (curpt == NULL || pts == NULL)
    +        return false;
     
         for (path_loop = 0; path_loop < pathc; path_loop++) {
             switch (pathop[path_loop].operation) {
    @@ -910,6 +912,8 @@ bool select_plotters(nsfb_t *nsfb)
     	free(nsfb->plotter_fns);
     
         nsfb->plotter_fns = calloc(1, sizeof(nsfb_plotter_fns_t));
    +    if (nsfb->plotter_fns == NULL)
    +        return false;
         memcpy(nsfb->plotter_fns, table, sizeof(nsfb_plotter_fns_t));
     
         /* set the generics */
    diff --git a/src/surface/ram.c b/src/surface/ram.c
    index 4deabda..f97224a 100644
    --- a/src/surface/ram.c
    +++ b/src/surface/ram.c
    @@ -38,6 +38,8 @@ static int ram_initialise(nsfb_t *nsfb)
         size_t size = (nsfb->width * nsfb->height * nsfb->bpp) / 8;
     
         nsfb->ptr = realloc(nsfb->ptr, size);
    +    if (!nsfb->ptr)
    +        return -1;
         nsfb->linelen = (nsfb->width * nsfb->bpp) / 8;
     
         return 0;
    @@ -68,6 +70,8 @@ static int ram_set_geometry(nsfb_t *nsfb, int width, int height, enum nsfb_forma
         endsize = (nsfb->width * nsfb->height * nsfb->bpp) / 8;
         if ((nsfb->ptr != NULL) && (startsize != endsize)) {
     	nsfb->ptr = realloc(nsfb->ptr, endsize);
    +	if (!nsfb->ptr)
    +	    return -1;
         }
         nsfb->linelen = (nsfb->width * nsfb->bpp) / 8;
     
    diff --git a/src/surface/wld.c b/src/surface/wld.c
    index 29f9ae2..b1c9916 100644
    --- a/src/surface/wld.c
    +++ b/src/surface/wld.c
    @@ -975,13 +975,14 @@ pointer_handle_motion(void *data,
         UNUSED(time);
     
         event = calloc(1, sizeof(struct wld_event));
    +    if (event) {
    +        event->event.type = NSFB_EVENT_MOVE_ABSOLUTE;
    +        event->event.value.vector.x = wl_fixed_to_int(sx_w);
    +        event->event.value.vector.y = wl_fixed_to_int(sy_w);
    +        event->event.value.vector.z = 0;
     
    -    event->event.type = NSFB_EVENT_MOVE_ABSOLUTE;
    -    event->event.value.vector.x = wl_fixed_to_int(sx_w);
    -    event->event.value.vector.y = wl_fixed_to_int(sy_w);
    -    event->event.value.vector.z = 0;
    -
    -    enqueue_wld_event(input->connection, event);
    +        enqueue_wld_event(input->connection, event);
    +    }
     }
     
     static void
    @@ -997,6 +998,8 @@ pointer_handle_button(void *data, struct wl_pointer *pointer, uint32_t serial,
         UNUSED(time);
     
         event = calloc(1, sizeof(struct wld_event));
    +    if (!event)
    +        return;
     
         if (state == WL_POINTER_BUTTON_STATE_PRESSED) {
     	event->event.type = NSFB_EVENT_KEY_DOWN;
    diff --git a/src/surface/x.c b/src/surface/x.c
    index f5ee01b..0e9b845 100644
    --- a/src/surface/x.c
    +++ b/src/surface/x.c
    @@ -846,10 +846,12 @@ static int x_initialise(nsfb_t *nsfb)
                            mask, values);
         /* set size hits on window */
         hints = xcb_alloc_size_hints();
    -    xcb_size_hints_set_max_size(hints, xstate->image->width, xstate->image->height);
    -    xcb_size_hints_set_min_size(hints, xstate->image->width, xstate->image->height);
    -    xcb_set_wm_size_hints(xstate->connection, xstate->window, WM_NORMAL_HINTS, hints);
    -    xcb_free_size_hints(hints);
    +    if (hints) {
    +        xcb_size_hints_set_max_size(hints, xstate->image->width, xstate->image->height);
    +        xcb_size_hints_set_min_size(hints, xstate->image->width, xstate->image->height);
    +        xcb_set_wm_size_hints(xstate->connection, xstate->window, WM_NORMAL_HINTS, hints);
    +        xcb_free_size_hints(hints);
    +    }
     
         /* create backing pixmap */
         xstate->pmap = xcb_generate_id(xstate->connection);
    -- 
    2.13.4
    
    
    patch file icon 0001-Always-check-return-value-for-alloc.patch (4,639 bytes) 2017-08-06 09:00 +

-Relationships
+Relationships

-Notes
Alastair Hughes

~0001577

Alastair Hughes (reporter)

I've attached a patch that should fix most of the segfaults that I was encountering in libnsfb; the indentation seems quite odd in the source so I've pretty much just left it using spaces. It hopefully illustrates the problem and the basic fixes.

There is a comment in the vnc backend suggesting that the library used doesn't check the return value from *alloc, so I've left it as is. The wld backend uses calloc to create new events - running out of memory in that case would be quite messy and probably needs a bit more attention than I've given it here.
Vincent Sanders

~0001643

Vincent Sanders (administrator)

Thankyou for your report, this has been resolved in the 3.7 release
+Notes

-Issue History
Date Modified Username Field Change
2017-08-06 04:38 Alastair Hughes New Issue
2017-08-06 09:00 Alastair Hughes File Added: 0001-Always-check-return-value-for-alloc.patch
2017-08-06 09:00 Alastair Hughes Note Added: 0001577
2017-09-09 16:46 Vincent Sanders Status new => acknowledged
2017-10-15 12:46 Vincent Sanders Assigned To => Vincent Sanders
2017-10-15 12:46 Vincent Sanders Status acknowledged => resolved
2017-10-15 12:46 Vincent Sanders Fixed in Version => 3.7
2017-10-16 22:33 Vincent Sanders Status resolved => closed
2017-10-16 22:33 Vincent Sanders Note Added: 0001643
+Issue History