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

serhiy-storchaka · 2017-11-01T10:00:36Z

Few bytes at the begin and at the end of the reallocated blocks, as well
as the header and the trailer, now are erased before calling realloc()
in debug build. This will help to detect using or double freeing the
reallocated block.

https://bugs.python.org/issue31626

Few bytes at the begin and at the end of the reallocated blocks, as well as the header and the trailer, now are erased before calling realloc() in debug build. This will help to detect using or double freeing the reallocated block.

vstinner · 2017-11-02T11:22:46Z

Objects/obmalloc.c

+    /* 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) {


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

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

vstinner · 2017-11-02T11:27:23Z

Objects/obmalloc.c

-        return NULL;
+    r = (uint8_t *)api->alloc.realloc(api->alloc.ctx, q - 2*SST, total);
+    if (r != NULL) {
+        q = r;


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.

vstinner · 2017-11-06T11:03:48Z

Objects/obmalloc.c

-        return NULL;
+    r = (uint8_t *)api->alloc.realloc(api->alloc.ctx, q - 2*SST, total);
+    if (r == NULL) {
+        nbytes = original_nbytes;


Please restore erased bytes here, and exit here.

For example, memcpy(q, save, Py_MIN(nbytes, original_nbytes)) below is wrong if realloc() failed: you must restore "original_nbytes" bytes, not Py_MIN(nbytes, original_nbytes) bytes.

This is why I have added nbytes = original_nbytes here. `memcpy(q, save, Py_MIN(nbytes, original_nbytes)) below is correct, it restores "original_nbytes" bytes.

Oh ok. But I still dislike all the code between realloc failure and code to restore erased byte executed even if realloc failed:

write_size_t(q, nbytes); q[SST] = (uint8_t)api->api_id; memset(q + SST + 1, FORBIDDENBYTE, SST-1); q += 2*SST; tail = q + nbytes; memset(tail, FORBIDDENBYTE, SST); write_size_t(tail + SST, serialno);

realloc() function is not supposed to modify anything if it fails.

I have duplicated the code as you suggested. But I don't like this. Now, if we will change something (for example will decide to save/restore bytes in the middle), we will need to change the code in three places instead of two.

vstinner · 2017-11-06T11:07:15Z

Objects/obmalloc.c

@@ -1517,11 +1517,13 @@ _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 */


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 ;-)

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.

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.

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

vstinner · 2017-11-06T11:09:07Z

Objects/obmalloc.c

    uint8_t *tail;        /* data + nbytes == pointer to tail pad bytes */
    size_t total;         /* 2 * SST + nbytes + 2 * SST */
    size_t original_nbytes;
-    int i;
+    size_t serialno;
+#define ERASED_SIZE 16


What do you think of increasing the size of the save buffer up to 512 bytes, so increase ERASED_SIZE to 256 bytes, to erase more bytes, and so catch more bugs?

At minimum, I propose to increa ERASED_SIZE to 64 bytes.

I afraid 512 bytes will increase the stack usage too much. Memory allocation can be used very deep in the call stack. Why you want to save more bytes? If the very start or the very end of the memory block are corrupted, this likely will crash the program, I think this is enough.

vstinner

Sorry, it's my fault, I misunderstood your code :-(

vstinner · 2017-11-06T14:05:15Z

Objects/obmalloc.c

+        memset(head + SST + 1, FORBIDDENBYTE, SST-1);
+
+        memset(tail, FORBIDDENBYTE, SST);
+        write_size_t(tail + SST, serialno);


Oh. We misunderstood each other. I asked why you would do that again if realloc() failed. I missed the fact that you erase the header and trailer... I understood that you only erased the start and end of the data block.

Now I understand why you didn't want to duplicate so much code, and I agree.

vstinner · 2017-11-06T14:16:54Z

Objects/obmalloc.c

+            memcpy(data + i, &save[ERASED_SIZE],
+                   Py_MIN(nbytes - i, ERASED_SIZE));
+        }
+        memcpy(data, save, Py_MIN(nbytes, ERASED_SIZE));


Would you mind to move this memcpy() before the if, to keep (start, end) order?

vstinner · 2017-11-06T14:18:47Z

Objects/obmalloc.c

+        }
+        memcpy(data, save, Py_MIN(nbytes, ERASED_SIZE));
+    }
+    _PyMem_DebugCheckAddress(api->api_id, data);


Hum, I'm not sure that it's useful to check the header and trailer since we just wrote them, no? Calling _PyMem_DebugCheckAddress() here can impact performance, no?

It was useful for writing this code. But since it has been written this check is not needed.

vstinner

LGTM.

Congrats for implementing the feature with your very long list of constraints (no additional heap allocation, "correct" code, low stack memory usage, etc.). Your code is very smart, and I missed some parts on my first rounds of review.

Do you want to backport this change to other Python branches?

I don't think that it's needed since the current code (modify freed memory after realloc) "just works". It does crash on OpenBSD, but I'm not interested to support OpenBSD in Python 2.7 and 3.6, I prefer to focus on supporting it in the master branch.

serhiy-storchaka · 2017-11-06T18:52:42Z

There was no crash on 2.7, and the fix for crash was backported to 3.6. This PR adds a new feature (the previous implementation didn't work, it was a dead code except OpenBSD where it caused a crash) that can help to catch memory bugs. I don't want to backport it.

vstinner · 2017-11-06T22:02:20Z

(...) the fix for crash was backported to 3.6. This PR adds a new feature (the previous implementation didn't work, (...)

Oh, I didn't see that you already removed the memset() from 3.6.

In that case, I would suggest to backport this PR to Python 3.6 as well, to not loose the detection of "use after free" by debug hooks.

serhiy-storchaka · 2017-11-06T22:34:15Z

Left this on to @ned-deily. But note that this detection doesn't work since 3.4, and this change is much more complex than #4145.

ned-deily · 2017-11-07T07:47:24Z

My reaction is that the risk due to complexity of the changes outweigh the benefits so I would agree with @serhiy-storchaka that we should not backport this.

vstinner · 2017-11-07T10:07:14Z

My reaction is that the risk due to complexity of the changes outweigh the benefits so I would agree with @serhiy-storchaka that we should not backport this.

Ok, fine.

vstinner · 2017-11-07T10:11:24Z

@serhiy-storchaka: For Python 3.6, what do you think of using Python 2.7 simple code?

...
    if (nbytes < original_nbytes) {
        /* shrinking:  mark old extra memory dead */
        memset(q + nbytes, DEADBYTE, original_nbytes - nbytes + 2*SST);
    }

    /* Resize and add decorations. We may get a new pointer here, in which
     * case we didn't get the chance to mark the old memory with DEADBYTE,
     * but we live with that.
     */
    q = (uchar *)PyObject_Realloc(q - 2*SST, total);
...

The code expect that realloc() never fails when shrinking. While Xavier De Gaye and me made realloc() failing anyway using tools to inject memory allocation failures, it seems like in the wild with system realloc(), realloc() never fails in that case.

If you agree, I can prepare a PR if you want. Or you can do, as you want :-)

For the master branch, I really prefer this PR since it's the "most correct" code: it supports hardcore testing as Xavier and me did ;-)

serhiy-storchaka · 2017-11-07T10:44:52Z

This is what the initial version of #3844 did. But I agree with your arguments, adding this in 3.6 is risky. The third-party code in 3.6 can expect that realloc() don't change the content on failure.

…#4210) Few bytes at the begin and at the end of the reallocated blocks, as well as the header and the trailer, now are erased before calling realloc() in debug build. This will help to detect using or double freeing the reallocated block.

serhiy-storchaka added the type-feature A feature request or enhancement label Nov 1, 2017

serhiy-storchaka requested a review from vstinner November 1, 2017 10:00

the-knights-who-say-ni added the CLA signed label Nov 1, 2017

bedevere-bot added the awaiting merge label Nov 1, 2017

serhiy-storchaka added the skip news label Nov 1, 2017

vstinner reviewed Nov 2, 2017

View reviewed changes

vstinner mentioned this pull request Nov 2, 2017

[WIP] bpo-31626: Rewrite _PyMem_DebugRawRealloc() #3898

Closed

Restore serialno and size when realloc failed.

1487108

vstinner reviewed Nov 6, 2017

View reviewed changes

serhiy-storchaka added 2 commits November 6, 2017 15:11

Merge branch 'master' into debug-realloc

9ec354b

Address review comments.

9f636df

vstinner reviewed Nov 6, 2017

View reviewed changes

Remove code duplications.

381150c

vstinner approved these changes Nov 6, 2017

View reviewed changes

serhiy-storchaka merged commit 3cc4c53 into python:master Nov 7, 2017

bedevere-bot removed the awaiting merge label Nov 7, 2017

serhiy-storchaka deleted the debug-realloc branch November 7, 2017 10:46

vstinner mentioned this pull request Nov 7, 2017

bpo-31626: Fix _PyObject_DebugReallocApi() #4310

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

serhiy-storchaka commented Nov 1, 2017 •

edited by bedevere-bot

vstinner Nov 2, 2017

serhiy-storchaka Nov 4, 2017

vstinner Nov 2, 2017

vstinner Nov 6, 2017

serhiy-storchaka Nov 6, 2017

vstinner Nov 6, 2017

serhiy-storchaka Nov 6, 2017

vstinner Nov 6, 2017

serhiy-storchaka Nov 6, 2017

vstinner Nov 6, 2017

serhiy-storchaka Nov 6, 2017

vstinner Nov 6, 2017

serhiy-storchaka Nov 6, 2017

vstinner left a comment

vstinner Nov 6, 2017

vstinner Nov 6, 2017

vstinner Nov 6, 2017

serhiy-storchaka Nov 6, 2017

vstinner left a comment

serhiy-storchaka commented Nov 6, 2017

vstinner commented Nov 6, 2017

serhiy-storchaka commented Nov 6, 2017 •

edited

ned-deily commented Nov 7, 2017

vstinner commented Nov 7, 2017

vstinner commented Nov 7, 2017

serhiy-storchaka commented Nov 7, 2017

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

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

Conversation

serhiy-storchaka commented Nov 1, 2017 • edited by bedevere-bot

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

serhiy-storchaka commented Nov 6, 2017

vstinner commented Nov 6, 2017

serhiy-storchaka commented Nov 6, 2017 • edited

ned-deily commented Nov 7, 2017

vstinner commented Nov 7, 2017

vstinner commented Nov 7, 2017

serhiy-storchaka commented Nov 7, 2017

serhiy-storchaka commented Nov 1, 2017 •

edited by bedevere-bot

serhiy-storchaka commented Nov 6, 2017 •

edited