bdc473369e1bb2a5666bd76626d449af50b81d40 braney Fri Jul 31 10:50:24 2026 -0700 resolve char * constants per file in the harvesters, not pooled refs #37925 refs #37923 Both harvesters merged file-scope "char *NAME = \"literal\";" definitions into the same table as the #defines, keyed by identifier with setdefault, so one program's private constant answered for every other file in the tree and whichever file os.walk reached first won. harvestCartVars.py already did this per file; these two hoisted it and broke. What it was getting wrong: - hgConfCatalog listed encpipeline_prod as an hg.conf variable, verified, cited at hgTracks.c:9867. That line is cfgNamesWithPrefix(database), the <db>_TopLink family, and encpipeline_prod is a hardcoded MySQL database name in hg/encode/docId/docIdView/docIdView.c. The row is gone and the call site now reads {database}, a runtime value. - Twelve URL records named the wrong variable. Five cartRemove(cart, varName) sites were reported as removing dnaLines, which is what an assembly tool at primeMate.c:296 calls its own varName; three cartRemove(cart, var) sites read as num. They now read {varName} and {var}. - cfgValsWithPrefix passing its own prefix parameter through resolved to an unrelated utility's "char *prefix = \"\";", and an empty string is falsy, so the call site was dropped instead of reported. It goes through NAME_SKIP now like the other accessor plumbing. The deliberate cost is the genuine cross-file constant: snp125ColorSourceOldVar is defined in hg/cgilib/snp125Ui.c and declared extern in hg/inc/snp125Ui.h, so its hgTrackUi call site now reads {snp125ColorSourceOldVar} rather than snp125ColorSource. Pooling only extern-declared names would recover it and bring the collisions straight back, since database is extern in hgTracks and separately initialized to a literal in that ENCODE tool. hgConfCatalog --check still reports 0 problems and --reconcile is unchanged; urlCommandCatalog --check still reports catalog ok with the persistence audit at 41 known leaks, 0 unrecorded, 0 stale. diff --git src/hg/utils/hgConfCatalog/harvestHgConf.py src/hg/utils/hgConfCatalog/harvestHgConf.py index df5594d19f2..a75c0a5c79a 100755 --- src/hg/utils/hgConfCatalog/harvestHgConf.py +++ src/hg/utils/hgConfCatalog/harvestHgConf.py @@ -116,100 +116,109 @@ "cfgOptionEnv": {"nameArg": 1, "defArg": None, "envArg": 0}, "cfgOptionEnvDefault": {"nameArg": 1, "defArg": 2, "envArg": 0}, "cfgOption2": {"twoPart": True, "defArg": None}, "cfgOptionDefault2": {"twoPart": True, "defArg": 2}, } # Reads whose name comes from a variable holding some other module's setting # name. These are the accessor's own plumbing in hgConfig.c, not settings. NAME_SKIP = {"name", "varName", "setting", "option", "prefix", "suffix"} CFG_CALL_RE = re.compile(r'\bcfg(?:Option[A-Za-z0-9]*|Val)\s*\(') PREFIX_CALL_RE = re.compile( r'\bcfg(?:Names|Vals)WithPrefix\s*\(\s*' r'("(?:[^"\\]|\\.)*"|[A-Za-z_]\w*)\s*\)') +# Not every settings name is a #define; some files hold one in a file-scope +# char * instead. These are resolved per file and never pooled, because the +# same identifier means different things in different programs: hgTracks passes +# a runtime `database` to cfgNamesWithPrefix, while docIdView.c has a +# file-scope `char *database = "encpipeline_prod"`. Pooling them let one +# program's constant answer for every other file, first one walked winning, and +# put a hardcoded MySQL database name in the registry as an hg.conf setting. +CONST_RE = re.compile( + r'^[ \t]*(?:static[ \t]+)?(?:const[ \t]+)?char[ \t]*\*[ \t]*([A-Za-z_]\w*)' + r'[ \t]*=[ \t]*("(?:[^"\\]|\\.)*")[ \t]*;', re.M) + # --------------------------------------------------------------------------- # macro table # --------------------------------------------------------------------------- def build_macros(): """Map every #define that resolves to a string literal, chasing aliases. Same two-pass approach as harvestUrlCommands.py: the tree defines names in terms of other names, so concatenating forms are resolved right to left over several rounds until nothing new appears. """ macro = {} chains = [] lit_re = re.compile( r'^\s*#\s*define\s+([A-Za-z_]\w*)\s+' r'("(?:[^"\\]|\\.)*")\s*(?:/[/*].*)?$') cat_re = re.compile( r'^\s*#\s*define\s+([A-Za-z_]\w*)\s+' r'((?:(?:"(?:[^"\\]|\\.)*")|(?:[A-Za-z_]\w*))' r'(?:\s+(?:(?:"(?:[^"\\]|\\.)*")|(?:[A-Za-z_]\w*)))*)' r'\s*(?:/[/*].*)?$') piece_re = re.compile(r'"(?:[^"\\]|\\.)*"|[A-Za-z_]\w*') - # Not every name is a #define. cart.c holds several of the central-table - # settings in file-scope char * variables instead. - const_re = re.compile( - r'^\s*(?:static\s+)?(?:const\s+)?char\s*\*\s*([A-Za-z_]\w*)\s*=\s*' - r'("(?:[^"\\]|\\.)*")\s*;') for fn in macro_files(): for line in open(fn, errors="replace"): m = lit_re.match(line) - if m: - macro.setdefault(m.group(1), m.group(2)[1:-1]) - continue - m = const_re.match(line) if m: macro.setdefault(m.group(1), m.group(2)[1:-1]) continue m = cat_re.match(line) if m: chains.append((m.group(1), piece_re.findall(m.group(2)))) for _ in range(5): for name, pieces in chains: if name in macro: continue out = "" for piece in pieces: if piece.startswith('"'): out += piece[1:-1] elif piece in macro: out += macro[piece] else: out = None break if out is not None: macro[name] = out return macro -def resolve(tok, macro): - """Turn one C token into the name it stands for, or {ident} if unknown.""" +def resolve(tok, macro, localconst=None): + """Turn one C token into the name it stands for, or {ident} if unknown. + + localconst is the file's own char * constants, which take precedence over + the shared #define table and must never be shared between files: see + CONST_RE for why. + """ tok = tok.strip() if not tok: return None if tok.startswith('"') and tok.endswith('"') and len(tok) >= 2: return tok[1:-1] if tok == "NULL": return None + if localconst and tok in localconst: + return localconst[tok] if tok in macro: return macro[tok] if re.match(r'^[A-Za-z_]\w*$', tok): return "{%s}" % tok return None # --------------------------------------------------------------------------- # file walking # --------------------------------------------------------------------------- def macro_files(): """Every .h and .c that could hold a #define we need to resolve.""" for d in MACRO_DIRS: p = os.path.join(ROOT, d) @@ -339,89 +348,94 @@ # --------------------------------------------------------------------------- # scanning # --------------------------------------------------------------------------- def scan_file(path, macro, found): try: raw = open(path, errors="replace").read() except OSError: return if "cfgOption" not in raw and "cfgVal" not in raw: return # hgConfig.c defines the accessors; its own calls are the implementation, # not settings reads. Its cfgVal/cfgOption uses inside other functions are # still real, so only the definitions are skipped, by name, below. text = strip_comments(raw) + localconst = {m.group(1): m.group(2)[1:-1] + for m in CONST_RE.finditer(text)} who = owner(path) for m in CFG_CALL_RE.finditer(text): op = m.end() - 1 fn = func_name(text, op) spec = ACCESSORS.get(fn) if spec is None: continue args = split_args(text, op) if not args: continue line = text.count("\n", 0, m.start()) + 1 src = "%s:%d" % (rel(path), line) default = None if spec.get("defArg") is not None and len(args) > spec["defArg"]: default = args[spec["defArg"]].strip() if spec.get("twoPart"): if len(args) < 2: continue # cfgOption2/cfgOptionDefault2 are themselves defined in terms of # each other in hgConfig.c, passing their own prefix/suffix # parameters through. Those are the implementation, not a read. if args[0] in NAME_SKIP or args[1] in NAME_SKIP: continue - pre = resolve(args[0], macro) - suf = resolve(args[1], macro) + pre = resolve(args[0], macro, localconst) + suf = resolve(args[1], macro, localconst) if pre is None or suf is None: continue if pre.startswith("{"): # runtime profile name: record the suffix as a family member found["profiles"][suf].append((pre, src, fn)) else: found["reads"][who].append( {"name": "%s.%s" % (pre, suf), "src": src, "func": fn, "default": default}) continue idx = spec["nameArg"] if len(args) <= idx: continue tok = args[idx] if tok in NAME_SKIP: continue - name = resolve(tok, macro) + name = resolve(tok, macro, localconst) if not name: continue rec = {"name": name, "src": src, "func": fn, "default": default} if spec.get("envArg") is not None and len(args) > spec["envArg"]: - env = resolve(args[spec["envArg"]], macro) + env = resolve(args[spec["envArg"]], macro, localconst) if env: rec["env"] = env if spec.get("boolean"): rec["boolean"] = True if spec.get("required"): rec["required"] = True found["reads"][who].append(rec) for m in PREFIX_CALL_RE.finditer(text): - name = resolve(m.group(1), macro) + if m.group(1) in NAME_SKIP: + # cfgValsWithPrefix passing its own parameter to cfgNamesWithPrefix + continue + name = resolve(m.group(1), macro, localconst) if name: line = text.count("\n", 0, m.start()) + 1 found["prefixScans"][name].append("%s:%d" % (rel(path), line)) def harvest(): macro = build_macros() found = {"reads": collections.defaultdict(list), "profiles": collections.defaultdict(list), "prefixScans": collections.defaultdict(list)} for path in source_files(): # The accessors themselves live here; their bodies read the config # hash directly and would otherwise show up as reads of {name}. if rel(path) in ("hg/lib/hgConfig.c", "hg/inc/hgConfig.h"): scan_file(path, macro, found)