Debug mode: enforce lifetime of char pointers returned from the API #259

steve-s · 2021-11-03T13:34:04Z

Implementation of #236.

~~This PR is built on top of #254. It probably makes sense to first review & merge that PR, or look at the diff the two PRs (available here: steve-s#1)~~

antocuni

Looks like a brilliant idea to me 💯

I think it deserves some better documentation though. I don't think we have user-facing docs for the debug mode yet, but for sure you should add some lenghty comment at the start of _debug_internal.h which explains the general idea of this check and how the various get_protected_raw_data_size & co. works, how/when the raw memory is allocated and freed, etc., because it's a bit hard to follow the logic by just reading the code.

Also, random idea for the future: I wonder whether we can write an implementation of raw_data_* which somehow integrates with AddressSanitizer and/or whether it would have any advantage over the current mmap-based implementation.

hpy/debug/src/_debugmod.c

antocuni · 2021-11-30T09:35:28Z

hpy/debug/src/debug_internal.h

+    // pointer to and size of any raw data associated with
+    // the lifetime of the handle:
+    void *associated_data;
+    HPy_ssize_t associated_data_size;


this looks certainly good for this specific PR.
However, we are wasting some space even for handles which don't require these fields (which are most of them).
For the future, we might want to design a sort of "DebugHandle subclassing" to make it possible to create different kinds of handles with different kinds of extra data, maybe?

Yes, I was concerned about this too. For now I don't see any other significantly better solution. I think the subclassing would not play well with the reusing of closed handles? In the future, in case we need to attach even more data to the debug handles, maybe we can use indirection with just one field in DebugHandle pointing to DebugHandleData or such and that would be allocated only if needed.

yes, if we do "subclassing" we would need to maintain a separate queue of closed handle for each different kind (or at least for each different sizeof()).
DebugHandleData works in theory but I don't know how good it is in practice, because you pay the cost of the pointer + the hidden space cost of an extra malloc (but I don't know how much they are).

Another idea to keep into consideration is to use a union if/when we will need more kind of data.

antocuni · 2021-11-30T09:38:42Z

hpy/debug/src/memprotect.c

+    }
+    return new_ptr;
+}
+


I think you addressed this concern in another comment but I wanted to double check. What happens if you mmap() lots of small buffers? The returned regions are aligned to page boundary (so, 4096 in general), but I assume that this consumes only the virtual address space and not the physical memory?

I think your concern is right. I think you cannot get smaller amount of physical memory than a page? I did not think about that.

We can introduce separate max number of the buffers alongside the max total size. On the other hand, the number of the buffers is limited by the max number of handles, so it should be fine I believe. The defaults are:

static const HPy_ssize_t DEFAULT_CLOSED_HANDLES_QUEUE_MAX_SIZE = 1024; static const HPy_ssize_t DEFAULT_PROTECTED_RAW_DATA_MAX_SIZE = 1024 * 1024 * 10;

if I see it correctly, currently the total size limit (10MB) is larger than if every of the 1024 handles had a tiny buffer attached (4MB for 4KB pages).

Moreover, thinking about this more: we always create the "debugging" copy of the data. The limits apply only to data attached to closed handles. This may mean that debug mode run may run out of memory when normal run wouldn't, but that may happen for other reasons too, so not sure if this is a big concern. The necessary amount of memory is "just" 2x, i.e., grows linearly, so I think it's fine?

I think you cannot get smaller amount of physical memory than a page?

indeed, I think you are correct.

On the other hand, the number of the buffers is limited by the max number of handles, so it should be fine I believe [cut]

yes, this looks correct as well. I vote for including your computation/explanation in the "big docstring which explains how it works".

This may mean that debug mode run may run out of memory when normal run wouldn't, but that may happen for other reasons too, so not sure if this is a big concern. The necessary amount of memory is "just" 2x, i.e., grows linearly, so I think it's fine?

yes, I think it's fine if debug mode requires more memory. Maybe at some point we can think of introducing optional checks (like, I want to enable leak checks but not raw buffers to save memory or so) but I don't think it's too important now.

antocuni · 2021-11-30T09:39:56Z

hpy/debug/src/memprotect.c

+        val = (val + 1) % 10;
+    }
+}
+


I wonder whether we could write something which is more readable in a debugger, like 0xDEADBEEF or "THIS MEMORY WAS FREED". But it's also true that this implementation will almost never be used in practice as linux is covered by the other one

Done. I went for 0xbad0da7a, because we want the data to be invalid when decoded. In the tests of this feature (test_charptr.py), when mmap is not available, we're now checking that the program fails with UnicodeDecodeError.

steve-s · 2021-11-30T12:45:04Z

Also, random idea for the future: I wonder whether we can write an implementation of raw_data_* which somehow integrates with AddressSanitizer and/or whether it would have any advantage over the current mmap-based implementation.

Right, the advantage would be that instead of just failing, we'd get a trace and all the info that AddressSanitizer provides. That would be cool. I could not find any API that AddressSanitizer would provide though. We would need to be able to mark some memory as "read only" and then as "invalid".

In general providing stack trace and some additional info would be useful for other debugging features like the invalid handle detection.

antocuni · 2021-11-30T14:38:37Z

Right, the advantage would be that instead of just failing, we'd get a trace and all the info that AddressSanitizer provides. That would be cool. I could not find any API that AddressSanitizer would provide though. We would need to be able to mark some memory as "read only" and then as "invalid".

yes but now that I think of it, this would work only if the user compiles its own code with ASan. In that case, it's probably enough to just free() the temporary buffer and let ASan to detect it as normal.
A good solution could be to offer different strategies to handle raw data so that the user can select the one which fits better his/her needs:

mmap()-based (if available)
"poisoning-based" (the default if mmap is not available)
free()-based (useful with ASan)
...

In general providing stack trace and some additional info would be useful for other debugging features like the invalid handle detection.

yes, that's also something which I wanted to add (and that should be optional since it's probably costly). Each DebugHandle should record the stacktrace of where it was created and/or closed.
Integrating with this PR should be "easy": we could install a fault handler, check whether the memory being accessed belongs to one of the closed handles and report it accordingly.
It's not necessary to do that in this PR of course, but could be a good idea to open an issue so that we remember the idea

steve-s · 2021-11-30T21:30:57Z

Last time I checked getting a stack trace in C in at least so-so portable way was not a simple thing to do. There are some libraries providing that functionality. What do you think about bringing some 3rd party dependency for that? We can probably make it at least optional with dlopen/dlsym instead of linking with it directly, or we can inline it, if it has appropriate license.

antocuni · 2021-12-01T09:53:38Z

Last time I checked getting a stack trace in C in at least so-so portable way was not a simple thing to do. There are some libraries providing that functionality. What do you think about bringing some 3rd party dependency for that? We can probably make it at least optional with dlopen/dlsym instead of linking with it directly, or we can inline it, if it has appropriate license.

yes, it's not portable and doesn't always work but that's life.
For linux, the simplest thing to do is to use backtrace which should be available everywhere. The most general solution is probably libunwind though. Whether to introduce a hard dependency or not, I don't know. It's probably better to discuss these details in an issue of its own than here, so we can make sure that the discussion will not be lost

Adds two fields to DebugHandle struct to keep any raw data associated with that handle and the size of the data. When the handle is closed, the associated data memory region is mprotected from reading/writing. AsUTF8AndSize returns a copy of the original result and saves that to the DebugHandle. The copy is also mprotected from reading.

Use _HPY_DEBUG_MEM_PROTECT_USEMMAP instead of _HPY_POSIX. Additionally allow to unconditionally disable _HPY_DEBUG_MEM_PROTECT_USEMMAP by exporting _HPY_DEBUG_FORCE_DEFAULT_MEM_PROTECT when building HPy.

… variable warning

steve-s · 2021-12-15T12:06:43Z

I think it deserves some better documentation though. I don't think we have user-facing docs for the debug mode yet, but for sure you should add some lenghty comment at the start of _debug_internal.h which explains the general idea of this check and how the various get_protected_raw_data_size & co. works, how/when the raw memory is allocated and freed, etc., because it's a bit hard to follow the logic by just reading the code.

this is done now

steve-s · 2021-12-15T13:38:08Z

@antocuni I believe I addressed all your comments. If you want, please, re-review the PR or let me know that you'd like to do so later. Otherwise I'd merge this next week.

steve-s mentioned this pull request Nov 3, 2021

Debug mode: enforce lifetime of char pointers returned from the API steve-s/hpy#1

Open

steve-s force-pushed the ss/char-ptrs-check-in-debug branch from de53f21 to 5b89537 Compare November 8, 2021 13:11

antocuni reviewed Nov 30, 2021

View reviewed changes

steve-s mentioned this pull request Dec 2, 2021

Provide better diagnostics for the debug mode #267

Open

steve-s added 12 commits December 13, 2021 16:21

Make TestCharPtr work on Windows

3b3e59c

Honor write_protect flag in UNIX raw_data_copy

96d7b34

Use more specific _HPY_DEBUG_MEM_PROTECT_USEMMAP

d835e66

Use _HPY_DEBUG_MEM_PROTECT_USEMMAP instead of _HPY_POSIX. Additionally allow to unconditionally disable _HPY_DEBUG_MEM_PROTECT_USEMMAP by exporting _HPY_DEBUG_FORCE_DEFAULT_MEM_PROTECT when building HPy.

Limit the size of the raw data intentionally leaked in debug mode

ae85663

Fix C memory leak in test_charptr_stress_test

a46fc20

Add more through test of leaked raw memory limit

4ba188d

get_closed_handles validates the max number of arguments

cf008ae

Non mmap based memprotect: override with 0xbad0da7a

70bb4fe

debug_handles.c: use as_DebugHandle(dh) in assertion to avoid unsused…

3e28f8d

… variable warning

Debug mode: improve the documentation of raw data protection

c6721e4

Make checks in test_charptr stricter

5805b1c

steve-s force-pushed the ss/char-ptrs-check-in-debug branch from 5b89537 to 5805b1c Compare December 15, 2021 12:02

steve-s added 2 commits December 15, 2021 13:42

Make test_charptr.py python 2.7 compatible

d8255e1

Make the clear_raw_data_in_closed_handles more reliable

40e9278

steve-s force-pushed the ss/char-ptrs-check-in-debug branch from 5e8e5a7 to 40e9278 Compare December 15, 2021 12:43

Tests: minor improvement in subprocess logging

d5e1f82

steve-s mentioned this pull request Dec 15, 2021

Extend debug tests (rebased) #254

Merged

steve-s merged commit df08977 into hpyproject:master Jan 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Debug mode: enforce lifetime of char pointers returned from the API #259

Debug mode: enforce lifetime of char pointers returned from the API #259

steve-s commented Nov 3, 2021 •

edited

antocuni left a comment

antocuni Nov 30, 2021

steve-s Nov 30, 2021

antocuni Nov 30, 2021

antocuni Nov 30, 2021

steve-s Nov 30, 2021

antocuni Nov 30, 2021

antocuni Nov 30, 2021

steve-s Dec 15, 2021

steve-s commented Nov 30, 2021

antocuni commented Nov 30, 2021

steve-s commented Nov 30, 2021

antocuni commented Dec 1, 2021

steve-s commented Dec 15, 2021

steve-s commented Dec 15, 2021

Debug mode: enforce lifetime of char pointers returned from the API #259

Debug mode: enforce lifetime of char pointers returned from the API #259

Conversation

steve-s commented Nov 3, 2021 • edited

antocuni 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

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

steve-s commented Nov 30, 2021

antocuni commented Nov 30, 2021

steve-s commented Nov 30, 2021

antocuni commented Dec 1, 2021

steve-s commented Dec 15, 2021

steve-s commented Dec 15, 2021

steve-s commented Nov 3, 2021 •

edited