| Title | MutatorFaultContext interface is inconsistent and incomplete |
| Status | closed |
| Priority | optional |
| Assigned user | Gareth Rees |
| Organization | Ravenbrook |
| Description | One of the tasks involved in porting the MPS to a new platform is to implement the Protection Mutator Context functional module (prmc). But this is difficult due to a number of inconsistencies and missing bits. See [1], [2], [3] for background. |
| Analysis | Here's an enumeration of the problems: 1. The functions ProtCanStepInstruction and ProtStepInstruction operate on a MutatorFaultContext but do not follow the usual naming convention (guide.implc.naming.prefix.program) by which they ought have names starting MutatorFaultContext. [Fixed in change 192528.] 2. There is no MutatorFaultContextCheck (see design.check.fun). [Fixed by change 192562 on OS X; change 192577 on POSIX/Linux; change 192567 on Windows; change 192583 on FreeBSD.] 3. suspendSignalHandler in pthrdext.c creates a MutatorFaultContext but only fills in the ucontext field (leaving the siginfo field uninitialized). There could be a MutatorFaultContextInit that ensures the contents are valid. [Fixed by changes 192562 and 192572 on OS X; change 192577 on POSIX/Linux; change 192567 on Windows.] 4. The Windows implementation of ThreadScan does not use a MutatorFaultContext and has no implementation of MutatorFaultContextScan. (It just calls GetThreadContext and scans the CONTEXT structure directly.) This means that the code for Windows is not parallel with the other platforms (making it harder to understand), and does not cope with capturing the context at one point and scanning it at a different point. [Fixed in change 192534.] 5. proti3.c and proti6.c are part of the protection mutator context module (not the protection module) and so should be named prmci3.c and prmci6.c. [Fixed in change 192512.] 6. The MutatorFaultContext* functions are declared in prot.h, but they should be declared in their own header, prmc.h. [Fixed in change 192528.] 7. The prmc*.c files are misnamed. Elsewhere in the project we put operating system before architecture (w3i6mv, xci6ll, etc.) so prmci3w3.c should be named prmcw3i3.c and so on. [Fixed in change 192512.] 8. There are implementations of the Prmci6* functions on LI, W3 and XC, but these are not used by proti6.c. If the idea is to keep them around in the expectation of eventually implementing ProtStepInstruction on x86-64, then we probably want to add the corresponding functions on FR for consistency. But it seems like a bad idea to keep code lying around that's unused and untested. Maybe better to delete these and resurrect them later if needed. [No need to do this now.] 9. global.c has a global variable MutatorFaultContext mps_exception_info which was added by Pekka Pirinen in change 21380 [4]. The comment says "This is a hack to make exception info easier to find in a release version" which I don't understand but possibly means it was intended to help with debugging, in which case surely it's not necessary any more. [Fixed in change 192518.] 10. MutatorFaultContext is poorly named because it isn't just used for faults. MutatorContext would be a better name. [Fixed in change 192523.] 11. "Protection mutator context" is also a poor name (the connection with protection is tenuous: protection faults are just one reason why you might need a mutator context). Plain "Mutator context" would be better. [Fixed in change 192528.] 12. Any platform using protsgix.c (currently FreeBSD) passes NULL as the context argument to ArenaAccess. This context may eventually gets passed to PoolSingleAccess, which calls MutatorContextCanStepInstruction, which may need to look at the context. Currently we get away with this because FreeBSD uses prmcan.c and so MutatorContextCanStepInstruction does not examine the context. It is not clear why protsgix.c does not create a MutatorContext in the same way as protli.c (using the ucontext_t passed to the signal handler). [Fixed by change 192550.] 13. If the problem in (12) above is fixed, then protli.c and protsgix.c will be essentially identical and could be merged into protsgix.c and then protli.c deleted. [Fixed by change 192556.] 14. The code in protsgix.c uses configuration parameters PROT_SIGNAL and PROT_SIGINFO_GOOD to make the code portable to other Unix platforms. But in fact POSIX specifies [5] that si_signo=SIGSEGV and si_code=SEGV_ACCERR have the meaning "Invalid permissions for mapped object" which is what we need. [Fixed by change 192556.] |
| How found | inspection |
| Evidence | [1] <https://info.ravenbrook.com/mail/2014/10/23/23-09-15/0/>[2] < https://info.ravenbrook.com/mail/2016/03/03/10-28-09/0/>[3] < https://info.ravenbrook.com/mail/2016/03/03/09-52-34/0/>[4] < https://info.ravenbrook.com/infosys/cgi/perfbrowse.cgi?@describe+21380>[5] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/signal.h.html |
| Created by | Gareth Rees |
| Created on | 2016-03-03 11:23:16 |
| Last modified by | Gareth Rees |
| Last modified on | 2016-10-14 13:26:15 |
| History | 2016-03-03 GDR Created. |
| Change | Effect | Date | User | Description |
|---|---|---|---|---|
| 192583 | closed | 2016-10-14 13:25:14 | Gareth Rees | Split generic mutator context module into two parts: one for generic operating system (prmcan.c) and one for generic architecture (prmcanan.c). |
| 192577 | open | 2016-10-14 12:39:50 | Gareth Rees | Add discriminator to MutatorContextStruct and implement MutatorContextInitFault and MutatorContextInitThread on POSIX/Linux. |
| 192572 | open | 2016-10-14 12:04:01 | Gareth Rees | Add discriminator to MutatorContextStruct and implement MutatorContextInitFault and MutatorContextInitThread on OS X. |
| 192567 | open | 2016-10-14 11:53:26 | Gareth Rees | New modulefile prmcw3.c avoids duplication of code between prmcw3i3.c and prmcw3i6.c. Implement MutatorContextCheck on Windows. Add AVERT(MutatorContext, context) in PoolAccess and other functions. Document initialization functions MutatorContextInitFault and MutatorContextInitThread and implement them on Windows. Add a union discriminator to MutatorContextStruct on Windows so that we don't accidentally try to get the stack pointer from a fault context, or the exception address from a thread context. |
| 192562 | open | 2016-10-13 23:13:40 | Gareth Rees | Initialization and checking of MutatorContext data structures. New files prmcix.c and prmcxc.c avoid duplicated code. |
| 192556 | open | 2016-10-13 21:24:04 | Gareth Rees | Use protsgix.c on Linux and delete protli.c. |
| 192550 | open | 2016-10-13 20:12:32 | Gareth Rees | In protsgix.c, construct a MutatorContext object and pass it to ArenaAccess. |
| 192534 | open | 2016-10-13 16:28:50 | Gareth Rees | Implement MutatorContextSP and MutatorContextScan for platforms w3i3 and w3i6. This means that ThreadScan becomes identical on these two platforms and can be moved to thw3.c. This means that thw3.h, thw3i3.c and thw3i6.c become redundant and can be deleted. |
| 192528 | open | 2016-10-13 15:06:14 | Gareth Rees | Rename the "protection mutator context" module to "mutator context" (this module handles mutator context decoding for both the protection module and the thread module). Rename functions Prot{Can,}StepInstruction to MutatorContext{Can,}StepInstruction so that they follow the naming convention in guide.implc.naming.prefix.program. Move mutator context declarations out of prot.h into new header prmc.h. Correct .assume.null in a couple of places -- it's not safe for MutatorContextStepInstruction to return ResUNIMPL, instead MutatorContextCanStepInstruction should return FALSE. |
| 192523 | open | 2016-10-13 14:23:39 | Gareth Rees | Rename MutatorFaultContext to MutatorContext because this data structure is not only used to store the context of a fault, but also to store the context of a thread that has been suspended. |
| 192518 | open | 2016-10-13 13:45:31 | Gareth Rees | Remove undocumented global mps_exception_info. Now that the MPS is open source, there is no difficulty in debugging the mutator context. |
| 192512 | open | 2016-10-13 13:24:47 | Gareth Rees | Rename prot{i3,i6}.c to prmc{i3,i6}.c because these files are part of the protection mutator context module. Rename prmc{i3,i6}{fr,li,w3,xc}.c to prmc{fr,li,w3,xc}{i3,i6}.c for consistency of ordering of platform codes (OS before AR before CT). |