Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

use better overflow check #2768

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

use better overflow check #2768

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 8 additions & 22 deletions src/lib/notcurses.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,29 +533,15 @@ make_ncpile(notcurses* nc, ncplane* n){
}


static inline size_t ncplane_sizeof_cellarray( unsigned rows, unsigned cols)
static inline size_t ncplane_sizeof_cellarray( size_t rows, size_t cols)
{
size_t fbsize;
size_t size;

// (nat) This protects against size_t overflow and also checks
// that dimensions are not zero en-passant. Why ?
// Assume: size_t is 16 bit, unsigned is 8 bit (UINT_MAX: 0xFF)
// nccell is sizeof( *p->fb): 0x10 (128 bit)
//
// 0xFF * 0xFF * 0x10 = 0xFE010, but overflows so: 0xE010
// 0x3F * 0x3F * 0x10 = 0xf810 is the biggest square possible

size = (size_t) cols * (size_t) rows;
if( size < (size_t) cols || size < (size_t) rows)
// check if multiplication would overflow
// calloc will deal with overflow due to sizeof(nccell)
if( ! rows || (cols > SIZE_MAX / rows))
return 0;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we'll want a diagnostic. see other uses of LOG_

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not easy to see from the way github presents these changes, but the return 0 will leas to:

  size_t fbsize = ncplane_sizeof_cellarray(p->leny, p->lenx);
  if(!fbsize || (p->fb = calloc(sizeof(struct nccell),fbsize)) == NULL){
    logerror("error allocating cellmatrix (r=%u, c=%u)",
             p->leny, p->lenx);
    free(p);
    return NULL;
  }

so there is a logerror there and the huge or zero values will provide ample clues as what went wrong. (IMO)


fbsize = sizeof( struct nccell) * size;
if( fbsize <= size)
return 0;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats deleted code though 😉


return fbsize;
return rows * cols;
}

// create a new ncplane at the specified location (relative to the true screen,
// having origin at 0,0), having the specified size, and put it at the top of
// the planestack. its cursor starts at its origin; its style starts as null.
Expand Down Expand Up @@ -615,8 +601,8 @@ ncplane* ncplane_new_internal(notcurses* nc, ncplane* n,
p->lenx = nopts->cols;
}

size_t fbsize = ncplane_sizeof_cellarray( p->leny, p->lenx);
if( ! fbsize || (p->fb = calloc(1,fbsize)) == NULL){
size_t fbsize = ncplane_sizeof_cellarray(p->leny, p->lenx);
if(!fbsize || (p->fb = calloc(sizeof(struct nccell),fbsize)) == NULL){
logerror("error allocating cellmatrix (r=%u, c=%u)",
p->leny, p->lenx);
free(p);
Expand Down
Loading