| Title | MPS poolams.c AMSFix omits checks: clientRef is within seg; bit index is valid |
| Status | closed |
| Priority | optional |
| Assigned user | Richard Brooksby |
| Organization | Ravenbrook |
| Description | MPS poolams.c AMSFix omits checks: clientRef is within seg; bit index is valid Related jobs: job001549 AMSFix assert !AMS_IS_INVALID_COLOUR(seg, i) RHSK 2006-12-14 AMSFix() just trusts that the *refIO passed in is in the seg passed in. It checks clientRef against SegBase. AMSFix should: 1. AVER that we are scanning at ambig before ignoring a clientRef into the format header. 2. check clientRef against seg limit. 3. bounds-check i = AMS_ADDR_INDEX(seg, base); |
| Analysis | RHSK 2006-12-14 Lack of AVER makes analysis of job001549 harder: AMSFix(): i = AMS_ADDR_INDEX(seg, base); AVER_CRITICAL(!AMS_IS_INVALID_COLOUR(seg, i)); ... [poolams.h] #define AMS_IS_INVALID_COLOUR(seg, index) \ (AMS_IS_GREY(seg, index) && !AMS_IS_WHITE(seg, index)) So: this grain is grey, but not white. Awwooga awwooga! Impl of grey and white: #define AMS_IS_WHITE(seg, index) \ (!BTGet(Seg2AMSSeg(seg)->nonwhiteTable, index)) #define AMS_IS_GREY(seg, index) \ (!BTGet(Seg2AMSSeg(seg)->nongreyTable, index)) Could this be reading outside the BT arrays? Yes: AMSFix does *not* bounds-check it. AMSFix: 1. 'trusts' that clientRef (*refIO of the refIO it is passed) is within the seg it is passed; 2. 'trusts' that i = AMS_ADDR_INDEX(seg, base) is a valid index; 3. just looks up that bit, whether within the nonXxxTables or not... Note that BT is a minimal type: typedef Word *BT. BT macros (or functions) cannot bounds-check index i. Why would poolams.c start getting this wrong after all this time? Perhaps the pervasive format->headerSize changes? Regardless of culpability here, index i should be bounds-checked in an AVER. |
| How found | unknown |
| Evidence | //info.ravenbrook.com/project/mps/master/code/poolams.c#14 |
| Observed in | 1.107.0 |
| Created by | Richard Kistruck |
| Created on | 2006-12-14 11:51:37 |
| Last modified by | Gareth Rees |
| Last modified on | 2014-04-10 18:16:29 |
| History | 2006-12-14 RHSK Created, inspired by job001549. 2006-12-28 RHSK Make title clearer. 2013-03-19 GDR Assigned to RB. 2013-06-16 RB Downgraded to "optional" as this isn't affecting anyone (see job003499). |
| Change | Effect | Date | User | Description |
|---|---|---|---|---|
| 185434 | closed | 2014-04-10 18:16:29 | Gareth Rees | Don't turn on the allocTable in AMSBufferEmpty when it's shared with nonwhiteTable and the colour tables are in use -- this will turn any grey grains in the segment invalid. Add more checking to AMS, including the table use invariant. |