Three separate bugs have now been fixed or filed for the same idiom: indexing x[strlen(x) - 1] without first checking x is non-empty. On an empty string the index is -1, one byte before the buffer.
Rather than wait for a fourth, here is the whole class as it stands on master. A sweep found 48 occurrences. Classifying by whether an emptiness guard appears within two lines: 32 guarded, 16 not, and 7 of the unguarded ones write:
WRITE htscore.c:2194 file[strlen(file) - 1] = '\0';
WRITE htscore.c:2242 file[strlen(file) - 1] = '\0';
WRITE htscoremain.c:368 tempo[strlen(tempo) - 1] = '\0';
WRITE htscoremain.c:874 tempo[strlen(tempo) - 1] = '\0';
WRITE htshelp.c:88 cmd[strlen(cmd) - 1] = '\0';
WRITE htslib.c:5496 fullpath[strlen(fullpath) - 1] = '\0';
WRITE htsname.c:441 fil_complete_patche[strlen(...) - 1] = '\0';
WRITE htsparse.c:2210 lien[strlen(lien) - 1] != '/'
read htshelp.c:87, htsname.c:518, :813, :1154, :1287, htsparse.c:1704, :2692
The two-line window makes "unguarded" mean candidate, not confirmed — several are likely protected by a check further up. That distance is the point: in #745 the bound came from a strlen(path) > HTS_URLMAXSIZE test dozens of lines above the write, which is why nobody reading the write could tell it was safe. The idiom keeps producing bugs because the guard is never where the index is.
Worth doing as one pass rather than one issue at a time: classify all 48, establish reachability where it exists, and convert the idiom to something that cannot be written wrongly — a small helper that returns the last character (or '\0' for an empty string) and one that strips a trailing separator in place. Both are three lines and remove the failure mode at the definition site instead of relying on every future caller remembering the guard.
Reachability matters for prioritisation but not for the fix: several of these take network-derived paths, and the ones that only ever see engine-built strings are still one refactor away from taking a hostile one.
Three separate bugs have now been fixed or filed for the same idiom: indexing
x[strlen(x) - 1]without first checkingxis non-empty. On an empty string the index is-1, one byte before the buffer.lienrelatif()— fixed,a75f437htsAddLink()— fixed, PR htsAddLink walks back from an empty codebase, one byte before the buffer #767url_savename()— open, and this one can write: if the byte before the buffer is/, the true branch stores a NUL thereRather than wait for a fourth, here is the whole class as it stands on master. A sweep found 48 occurrences. Classifying by whether an emptiness guard appears within two lines: 32 guarded, 16 not, and 7 of the unguarded ones write:
The two-line window makes "unguarded" mean candidate, not confirmed — several are likely protected by a check further up. That distance is the point: in #745 the bound came from a
strlen(path) > HTS_URLMAXSIZEtest dozens of lines above the write, which is why nobody reading the write could tell it was safe. The idiom keeps producing bugs because the guard is never where the index is.Worth doing as one pass rather than one issue at a time: classify all 48, establish reachability where it exists, and convert the idiom to something that cannot be written wrongly — a small helper that returns the last character (or
'\0'for an empty string) and one that strips a trailing separator in place. Both are three lines and remove the failure mode at the definition site instead of relying on every future caller remembering the guard.Reachability matters for prioritisation but not for the fix: several of these take network-derived paths, and the ones that only ever see engine-built strings are still one refactor away from taking a hostile one.