few realloc calls changes and overflow checks related. #2162

devnexen · 2016-10-15T13:54:47Z

No description provided.

KalleZ · 2016-10-15T17:04:43Z

ext/opcache/zend_accelerator_blacklist.c

+
 		blacklist->size += ZEND_BLACKLIST_BLOCK_SIZE;
-		blacklist->entries = (zend_blacklist_entry *) realloc(blacklist->entries, sizeof(zend_blacklist_entry)*blacklist->size);
+		zend_blacklist_entry * entries = (zend_blacklist_entry *) realloc(blacklist->entries, sizeof(zend_blacklist_entry) * blacklist->size);


This won't work on C89, as decls must be first in a block, sadly, Windows is one such case

KalleZ · 2016-10-15T17:06:45Z

ext/xmlrpc/libxmlrpc/base64.c

+    }
    b->length += 512;
-    b->data = realloc(b->data, b->length);
+    char *data = realloc(b->data, b->length);


Same as above comment, decls first in block

KalleZ · 2016-10-15T17:07:39Z

ext/xmlrpc/libxmlrpc/simplestring.c

+                return;
         }
-         target->str = (char*)realloc(target->str, newsize);
+         char *str = (char*)realloc(target->str, newsize);


Same as above comment, decls first in block

KalleZ · 2016-10-15T17:07:58Z

main/network.c

+		}
 		*hstbuflen *= 2;
-		*tmphstbuf = (char *)realloc (*tmphstbuf,*hstbuflen);
+		char *ptmphstbuf = (char *)realloc(*tmphstbuf, *hstbuflen);


Same as above comment, decls first in block

KalleZ · 2016-10-15T17:08:14Z

main/network.c

+		}
 		*hstbuflen *= 2;
-		*tmphstbuf = (char *)realloc (*tmphstbuf,*hstbuflen);
+		char *ptmphstbuf = (char *)realloc(*tmphstbuf, *hstbuflen);


Same as above comment, decls first in block

KalleZ · 2016-10-15T17:08:37Z

main/network.c

 		if (*hstbuflen < sizeof(struct hostent_data)) {
 			*hstbuflen = sizeof(struct hostent_data);
-			*tmphstbuf = (char *)realloc(*tmphstbuf, *hstbuflen);
+			char *ptmphstbuf = (char *)realloc(*tmphstbuf, *hstbuflen);


Same as above comment, decls first in block

nikic · 2016-10-15T17:29:03Z

main/network.c

 		&& (errno == ERANGE)) {
+		char *ptmphstbuf;
 		/* Enlarge the buffer. */
+		if (*hstbuflen * 2 >= SIZE_MAX) {


This condition is always false. Should probably be *hstbuflen > SIZE_MAX / 2? (Same below)

krakjoe · 2016-10-20T04:37:29Z

Can I get an update on status here please ?

Should be rebased on 7.0, master is using zend MM in libxmlrpc.

nikic · 2016-10-20T10:27:50Z

Zend/zend_alloc.c

+		p = pp;
 		return p;
 	}
+	free(p);


These changes aren't necessary -- zend_out_of_memory will exit anyway. We can't avoid "leaks" in that situation in any case (where leak = reachable memory).

nikic · 2016-10-20T10:46:16Z

I'm not a huge fan of checking alloc return values -- it makes the code pretty ugly to handle cases that are not really of practical relevance. I wonder if it wouldn't be better to replace something like

char *data = realloc(b->data, b->length);
if (!data) {
    buffer_delete(b);
    return;
}
b->data = data;

with

b->data = perealloc(b->data, b->length, 1);

perealloc in persistent mode goes through __zend_realloc, which is infallible, so we don't need to deal with the failure case. I'm seeing a bunch of places using pemalloc functions with a hardcoded 1 argument, so it seems like we already use this kind of pattern.

nikic · 2016-10-20T10:49:38Z

sapi/cli/php_cli.c

+			if (!pini_entries) {
+				goto out;
+			}
+			ini_entries = pini_entries;


Using the suggestion from #2162 (comment), this whole change (including the overflow check) could be written as:

ini_entries = safe_perealloc(ini_entries, ini_entries_len, 1, sizeof(HARDCODED_INI), 1);

perealloc handles the failure case, safe handles the overflow.

nikic · 2016-10-20T10:52:01Z

sapi/cli/php_cli.c

 					val++;
 					if (!isalnum(*val) && *val != '"' && *val != '\'' && *val != '\0') {
-						ini_entries = realloc(ini_entries, ini_entries_len + len + sizeof("\"\"\n\0"));
+						if ((ini_entries_len + len + sizeof("\"\"\n\0")) >= SIZE_MAX) {


Generally all checks of the form >= SIZE_MAX are ineffective, because the overflow will already happen before the comparison.

krakjoe · 2017-04-03T07:26:22Z

I think this was superseded by work done by Nikita, so closing this PR.

few realloc calls changes and overflow checks related.

7ed4b94

KalleZ reviewed Oct 15, 2016

View reviewed changes

KalleZ suggested changes Oct 15, 2016

View reviewed changes

declare variables beginning of blocks

92f301b

nikic reviewed Oct 15, 2016

View reviewed changes

lowering overflow threshold

67d15a1

nikic reviewed Oct 20, 2016

View reviewed changes

smalyshev added the Bug label Oct 31, 2016

krakjoe added the Waiting on Author label Jan 3, 2017

krakjoe closed this Apr 3, 2017

few realloc calls changes and overflow checks related. #2162

few realloc calls changes and overflow checks related. #2162

Uh oh!

Conversation

devnexen commented Oct 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krakjoe commented Oct 20, 2016

Uh oh!

nikic Oct 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic commented Oct 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic Oct 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krakjoe commented Apr 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nikic Oct 20, 2016 •

edited

Loading

nikic commented Oct 20, 2016 •

edited

Loading

nikic Oct 20, 2016 •

edited

Loading