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.

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

use better overflow check #2768

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

Conversation

mulle-nat
Copy link
Contributor

This is a better mergable #2767, but my fork got out of sync and there were superflous commits in there. Sorry for the inconvenience. Here's my original comment.

I think you will like this much better. The other code was more or less a mitigation but this
should be a better overflow checker. The multiplication does the zero check and the
comparison checks that the multiplication doesn't overflow. calloc will then do the other
overflow check.

Adding to this: The old code caught overflow with large negative numbers, but this one will catch any kind of overflow. Also the code is easier to understand IMO.

@dankamongmen
Copy link
Owner

if we're really committing to this catch overflow idea, don't we want it backing ncplane_create(), and thus handling all ncplane creations?

@dankamongmen dankamongmen self-assigned this Mar 3, 2024
@dankamongmen
Copy link
Owner

if we're really committing to this catch overflow idea, don't we want it backing ncplane_create(), and thus handling all ncplane creations?

nevermind, i think it is by virtue of being in ncplane_new_internal()

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 😉

@mulle-nat
Copy link
Contributor Author

Just wondering, if I need to add some more comments here, or if everything is clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants