Castlequest icon indicating copy to clipboard operation
Castlequest copied to clipboard

Guard against using OBJECT=0 as array index

Open apthorpe opened this issue 4 years ago • 2 comments

This should trap any instance of OBJECT being used as an array index in main when OBJECT is zero. The original user experience is preserved but it allows the code to be built with -fcheck=bounds

apthorpe avatar Apr 15 '21 15:04 apthorpe

@apthorpe: Thanks very much for this patch! It's right at the boundary between "patches I want to accept" and "patches I don't." On the one hand it's kind of invasive (multiple scattered lines changed), and in a way that's not obviously non-original if you're not looking at the git history. On the other hand, it seems to be as minimal as possible — it's just solving a problem whose solution is inherently a bit invasive. And it is solving a problem, a UB in the original code; but on the other hand, that's not a major problem, and not a problem at all for the average player (because the average player doesn't go exploring what happens when you build with -fcheck=bounds).

For now I'm going to not merge this patch; but leave it open; but also put it way at the bottom of my list of things to think about.

Quuxplusone avatar Apr 16 '21 16:04 Quuxplusone

Fair enough. It was written assuming ITEMS(OBJECT) returned 0 for OBJECT = 0 and replicated the behavior of the later code that segfaults because of bad indexing; the behavior should be the same as the original code.

This is sort of a rare condition. You only see the segfault issue if you if you enable bounds checking and you enter specific verbs without a noun.

Again, I totally understand your position on invasive patches so it's not like I'm codehurt over rejection :) Consider this to be an "Oh, by the way...." if or when you do a rewrite or if you see any user reports about this issue (doubtful but possible).

Other trivia: I found a reference for all those commented-out FTNCMD lines in init.f and savres.f - was this code originally written for the Michigan Terminal System? I'm trying to identify the origin of FTNCMD and all I've found so far is http://www.bitsavers.org/pdf/univOfMichigan/mts/volumes/MTSVol06-FortranInMTS-Feb1988.pdf I'm trying to determine if FTNCMD is an IBM extension or a MTS extension because I'm cataloguing legacy Fortran idioms (a long story I'm trying to get published). If I find the answer, I'll let you know...

Again, thanks for taking the time to look it over. I'm making good progress on my conversion to Fortran 2018. If I find any more potentially game-breaking weirdness, I'll pass it along as a courtesy.

apthorpe avatar Apr 17 '21 15:04 apthorpe