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

bpo-31626: Mark ends of the reallocated block in debug build. #4210

Merged
merged 5 commits into from Nov 7, 2017
Merged

bpo-31626: Mark ends of the reallocated block in debug build. #4210

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a5a5db4
bpo-31626: Mark ends of the reallocated block in debug build.
serhiy-storchaka Nov 1, 2017
1487108
Restore serialno and size when realloc failed.
serhiy-storchaka Nov 4, 2017
9ec354b
Merge branch 'master' into debug-realloc
serhiy-storchaka Nov 6, 2017
9f636df
Address review comments.
serhiy-storchaka Nov 6, 2017
381150c
Remove code duplications.
serhiy-storchaka Nov 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
58 changes: 44 additions & 14 deletions Objects/obmalloc.c
View file
Open in desktop
Expand Up @@ -1517,11 +1517,12 @@ _PyMem_DebugRawRealloc(void *ctx, void *p, size_t nbytes)

debug_alloc_api_t *api = (debug_alloc_api_t *)ctx;
uint8_t *q; /* base address of malloc'epad d block */
uint8_t *data; /* p + 2*SST == pointer to data bytes */
Copy link
Member

Choose a reason for hiding this comment

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

I added the data variable to make the code easier to follow and understand.

With "q += 2*SST;" in the middle of the function, it's hard to follow which bytes are modified in the buffer.

Using the data variable, you can use my ASCII-art schema in _PyMem_DebugRawAlloc() to see what the memory block contains.

Please keep my data variable ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

I restored the using of the q variable for symmetry in saving/restoring code. Do you prefer destroying the symmetry or using the data variable in saving too? Using the data variable will make one line too long, it should be splitted on two lines.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I don't understand your symmetry question. I would like to have similar code in _PyMem_DebugRawAlloc() and _PyMem_DebugRawRealloc(). With "q += 2*SST;", the q documentation is wrong:

    uint8_t *q;           /* base address of malloc'epad d block */

With "q += 2*SST;", the meaning of q depends where your are in the function. For me, it's hard to follow :-( The memory block layout is complex.

Copy link
Member Author

Choose a reason for hiding this comment

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

The documentation was wrong before. q is not a base address of malloc'ed d block before calling realloc().

uint8_t *r;
uint8_t *tail; /* data + nbytes == pointer to tail pad bytes */
size_t total; /* 2 * SST + nbytes + 2 * SST */
size_t original_nbytes;
int i;
#define ERASED_SIZE 8
uint8_t save[2*ERASED_SIZE]; /* A copy of erased bytes. */

_PyMem_DebugCheckAddress(api->api_id, p);

Expand All @@ -1533,31 +1534,60 @@ _PyMem_DebugRawRealloc(void *ctx, void *p, size_t nbytes)
}
total = nbytes + 4*SST;

tail = q + original_nbytes;
/* Mark the header, the trailer, ERASED_SIZE bytes at the begin and
ERASED_SIZE bytes at the end as dead and save the copy of erased bytes.
*/
if (original_nbytes <= 2*ERASED_SIZE) {
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to use sizeof(save) in the test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you prefer to use sizeof(save)/2 instead of ERASED_SIZE?

memcpy(save, q, original_nbytes);
memset(q - 2*SST, DEADBYTE, original_nbytes + 4*SST);
}
else {
memcpy(save, q, ERASED_SIZE);
memset(q - 2*SST, DEADBYTE, ERASED_SIZE + 2*SST);
memcpy(&save[ERASED_SIZE], tail - ERASED_SIZE, ERASED_SIZE);
memset(tail - ERASED_SIZE, DEADBYTE, ERASED_SIZE + 2*SST);
}

/* Resize and add decorations. */
q = (uint8_t *)api->alloc.realloc(api->alloc.ctx, q - 2*SST, total);
if (q == NULL) {
return NULL;
r = (uint8_t *)api->alloc.realloc(api->alloc.ctx, q - 2*SST, total);
if (r != NULL) {
q = r;
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to restore bytes and exit from here. It seems strange to increase the serial number, call write_size_t(q, nbytes), etc. if realloc failed.

Restoring bytes here would make the code simpler, since you don't have to use Py_MIN() here.

}

bumpserialno();
write_size_t(q, nbytes);
assert(q[SST] == (uint8_t)api->api_id);
for (i = 1; i < SST; ++i) {
assert(q[SST + i] == FORBIDDENBYTE);
}
data = q + 2*SST;
q[SST] = (uint8_t)api->api_id;
memset(q + SST + 1, FORBIDDENBYTE, SST-1);
q += 2*SST;

tail = data + nbytes;
tail = q + nbytes;
memset(tail, FORBIDDENBYTE, SST);
write_size_t(tail + SST, _PyRuntime.mem.serialno);

/* Restore saved bytes. */
if (original_nbytes <= 2*ERASED_SIZE) {
memcpy(q, save, Py_MIN(nbytes, original_nbytes));
}
else {
size_t i = original_nbytes - ERASED_SIZE;
if (nbytes > i) {
memcpy(q + i, &save[ERASED_SIZE], Py_MIN(nbytes - i, ERASED_SIZE));
}
memcpy(q, save, Py_MIN(nbytes, ERASED_SIZE));
}
_PyMem_DebugCheckAddress(api->api_id, q);

if (r == NULL) {
return NULL;
}

if (nbytes > original_nbytes) {
/* growing: mark new extra memory clean */
memset(data + original_nbytes, CLEANBYTE,
nbytes - original_nbytes);
memset(q + original_nbytes, CLEANBYTE, nbytes - original_nbytes);
}

return data;
return q;
}

static void
Expand Down