MPS issue job001550

TitleMPS poolams.c AMSFix omits checks: clientRef is within seg; bit index is valid
Statusclosed
Priorityoptional
Assigned userRichard Brooksby
OrganizationRavenbrook
DescriptionMPS 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);
AnalysisRHSK 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 foundunknown
Evidence//info.ravenbrook.com/project/mps/master/code/poolams.c#14
Observed in1.107.0
Created byRichard Kistruck
Created on2006-12-14 11:51:37
Last modified byGareth Rees
Last modified on2014-04-10 18:16:29
History2006-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).

Fixes

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.