check_data() may use uninitialized variable (trac #394)

[cloudy-bot]

reported by: peter

The routine check_data() suppresses cautions about md5sum mismatches when the NO TIME command is in effect. Hence it reads the variable prt.lgPrintTime. This variable is first set in the routine !InitDefaultsPreparse(). The problem is that by then some data files have already been read and check_data() will have read the "uninitialized" version of prt.lgPrintTime (actually since the data is global, it will have been set to zero / false by default which is the wrong initialization).

There are two solutions.

1) give t_prt a ctor that sets lgPrintTime. This guarantees consistent behavior when the NO TIME command is not in effect, but not when the NO TIME command is used. The latter is a problem because some files are read before the parsing starts, leading to inconsistent behavior of check_data() before and after parsing.

2) revert the change to check_data() and give a caution despite the NO TIME command. This way check_data() no longer needs to access prt.lgPrintTime. However, this will lead to more frequent breaking of the repeatability tests whenever a datafile on a release branch has been updated without updating the md5sum file.

I am not sure yet which is the better solution...

Migrated from https://www.nublado.org/ticket/394

{
    "status": "closed",
    "changetime": "2019-02-04T12:09:37Z",
    "_ts": "1549282177514312",
    "description": "The routine check_data() suppresses cautions about md5sum mismatches when the NO TIME command is in effect. Hence it reads the variable prt.lgPrintTime. This variable is first set in the routine !InitDefaultsPreparse(). The problem is that by then some data files have already been read and check_data() will have read the \"uninitialized\" version of prt.lgPrintTime (actually since the data is global, it will have been set to zero / false by default which is the wrong initialization).\n\nThere are two solutions.\n\n1) give t_prt a ctor that sets lgPrintTime. This guarantees consistent behavior when the NO TIME command is not in effect, but not when the NO TIME command is used. The latter is a problem because some files are read before the parsing starts, leading to inconsistent behavior of check_data() before and after parsing.\n\n2) revert the change to check_data() and give a caution despite the NO TIME command. This way check_data() no longer needs to access prt.lgPrintTime. However, this will lead to more frequent breaking of the repeatability tests whenever a datafile on a release branch has been updated without updating the md5sum file.\n\nI am not sure yet which is the better solution...",
    "reporter": "peter",
    "cc": "",
    "resolution": "fixed",
    "time": "2017-07-19T15:03:46Z",
    "component": "infrastructure",
    "summary": "check_data() may use uninitialized variable",
    "priority": "minor",
    "keywords": "",
    "version": "trunk",
    "milestone": "c17.01",
    "owner": "nobody",
    "type": "defect - etc"
}

[cloudy-bot]

• @peter-van-hoof-noaccount commented:

Solution 1 was chosen, with the additional change that the NO TIME command is now picked up in "preparsing" in cdDrive(), making it far less likely that a data file is read before this command is picked up.

Fixed on the trunk in r11806/7, and r11808 on c17_branch.

• @peter-van-hoof-noaccount changed resolution from "" to "fixed"
• @peter-van-hoof-noaccount changed status from "new" to "closed"

[cloudy-bot]

• @CloudyLex commented:

Milestone renamed

• @CloudyLex changed milestone from "c17.01 release" to "C17.01 release"

[cloudy-bot]

• @CloudyLex commented:

Milestone renamed

• @CloudyLex changed milestone from "C17.01 release" to "C17.01"

[cloudy-bot]

• @peter-van-hoof-noaccount commented:

Milestone renamed

• @peter-van-hoof-noaccount changed milestone from "C17.01" to "c17.01"