<i>Very rare bug (segfault) if set variable (with error case) using self-releasable object as new value</i> status still Pending with 3 other changes
by
pooryorick
2023-03-30 19:01:35.
D 2023-03-30T19:01:35.580
J icomment While\sdiagnosing\sand\sfixing\s[6d4e9d1af5|memory\sleak:\sSetFsPathFromAny,\sassisted\r\nby\sthe\sglobal\sliteral\stable,\scauses\sa\sTcl_Obj\sto\sreference\sitself],\sI\sspent\r\ndays\slooking\sinto\sthis\sand\ssimilar\sreports.\s\sI\stried\svarious\sexperiments\sto\r\ncome\sup\swith\san\sexample\sthat\swould\sconfirm\sthis\sreport,\sultimately\sspending\sa\r\nfair\schunk\sof\stime\sonly\sto\sdetermine\sthat\sit\sis\sbaseless.\s\sNow\sit's\stime\sto\sput\r\nthis\sticket\sto\srest\sso\sthat\sthe\snext\sdeveloper\sdoesn't\shave\sto\sspend\stheir\stime\r\non\sit\stoo.\s\s[510663a99e3a096b]\sis\sa\ssmart\scommit\sand\sa\sgood\scommit:\s\sIt\sfixes\r\nthe\sreal\sbug,\sas\sdescribed\sby\smsofer\sfor\s[1334947]:\s\sThe\sreal\sbug\sis\sthat\snot\r\nall\sfailure\spaths\sclear\s0-ref\sobjs.\s\sIt\salso\smade\sTc_ObjSetVar2\sdo\smore\sso\sthat\r\nthe\scaller\scan\sdo\sless.\s\sThe\s"TCL_OWN_OBJREF"\sinvention\sproposed\sby\ssebres\r\ndoesn't\ssolve\sany\sknown\sproblem\sso\sthere\sis\sno\sreason\sto\sadopt\sit.\s\sThe\r\nad-hominem\sassertion,\s"someone\sdoesn't\sunderstand\ssomething\s(or\sdoesn't\swant\r\nspent\smore\stime\sto\sgo\sdeeper)"\sis\ssimply\sanother\sindicator\sthat\sthere\sis\sno\r\nsubstantial\sargument\sto\sbe\smade\shere\son\sbehalf\sof\sthis\sreport.\r\n\r\nNow\sfor\sa\sbasic\sexplanation\sof\sreference\scounting\sin\sTcl:\sIt\sis\swell\sdocumented\r\nand\swell\sunderstood\sthat\sin\sorder\sto\spreserve\sa\sTcl_Obj\sa\scaller\sincrements\sits\r\nreference\scount\sbefore\spassing\sit\sto\sanother\sprocedure,\sand\slater\seither\r\ndecrements\sthe\sreference\scount\sor\sdocuments\sthat\sit\shas\sincremented\sthe\r\nreference\scount\sso\sthat\sits\sown\scaller\scan\sdecrement\sit.\s\sIt's\scommon\sfor\r\nprocedures\sto\sincrement\sand\sthen\sdecrement\sa\sTcl_Obj\sreference\scount,\sand\r\nTcl_ObjSetVar2\sdoes\snothing\smore\sthan\sthe\sequivalent\sof\sthat,\sand\sits\snot\r\nalone.\s\sIf\sa\sprocedure\sknows\sthat\snothing\selse\sis\sgoing\sto\sdecrement\sthe\r\nreference\scount\sin\sthe\smeantime,\sthen\sthe\sperformant\sway\sto\sdo\sthe\sequivalent\r\nof\sincrementing\sand\sthen\sdecremening\sthe\sreference\scount\sis\sto\sonly\sincrement\r\nit\son\ssuccess,\sand\sthen\son\sfailure\sto\shandle\sthe\sonly\scase\swhere\sdecrementing\r\nit\swould\sdo\sanything:\s\sThe\scase\sin\swhich\sthe\sreference\scount\sis\szero.\s\sThat's\r\nall\sthat\sTcl_ObjSetVar2\sdoes.\s\sIt's\selegant\sand\scorrect.\r\n\r\nNow\sto\srebut\sthe\sstatements\smade\sby\ssebres\sso\sfar:\r\n\r\n"we\scan\sbe\snot\ssure\swhat\shappens\swith\sreference\sin\sTcl_ObjSetVar2"\r\n\r\n<blockquote>\r\nYes,\swe\scan\sbe\ssure:\s\sThe\sreference\scount\sof\sthe\sTcl_Obj\sis\sincremented,\sand\sin\r\nthe\sfailure\scase,\sit\sis\sdecremented.\r\n</blockquote>\r\n\r\n"The\sfunction\schecks\sthe\srefCount\sonly\sAFTER\strying\sof\sset\sthe\svariable\s(which\r\ncan\schange\sthe\srefCount\sof\sthe\sobject\sby\scalling\sof\sthe\straces..."\r\n\r\n<blockquote>\r\nFalse.\s\sAs\sdgp\sexplained,\sin\sthe\sfailure\scase\sTcl_ObjSetVar2\sdecrements\sthe\sreference\scount\sand\sreturns\sbefore\sany\strace\sruns.\r\n</blockquote>\r\n\r\n"Much\sworse\sit\slooks\sjust\slike\sundefined\sbehavior."\r\n\r\n<blockquote>\r\nFalse.\s\sTcl_ObjSetVar2\sperforms\sthe\sequivalent\sof\sincrementing\sthe\svariable,\r\nand\sthen\sdecrementing\sit\sin\sthe\serror\scase.\r\n</blockquote>\r\n\r\n\r\n"For\sthe\sdeveloper\susing\sTcl_ObjSetVar,\sit\sis\svery\sbad\sup\sto\simpossible\sto\sact\splausible\sin\serror\scase,\sbecause\she\sdon't\sknow\swhen\she\sshould\sdecrease\srefCount\sin\serror\scase\sor\snot..."\r\n\r\n<blockquote>\r\nFalse.\s\sThe\scaller\seither\sincrements\sthe\sTcl_Obj\sprior\sto\spassing\sit\sto\sTcl_ObjSetVar2\sand\sarranges\sfor\sit\sto\sbe\sdecremented\slater,\sor\sjust\sdoesn't\sincrement\sit\sand\sallows\sTcl_ObjSetVar2\sto\smanage\sit.\r\n</blockquote>\r\n\r\n"Thus\seither\she\sdo\sthis\snevertheless\sand\scan\sget\ssegfault,\sor\she\sproduces\sintentionally\sa\smemory\sleak\s(quasi\saccepts\sthe\srisk\sof\spossible\sleak)."\r\n\r\n<blockquote>\r\nFalse.\s\sIf\sthe\scaller\swants\sto\suse\sthe\sTcl_Obj\safter\spasssing\sit\sto\r\nTcl_ObjSetVar2,\sit\ssimply\sincrements\sthe\sreference\scount\sfirst,\sand\sthen\r\ndecrements\sit\slater,\sor\spasses\sit\sback\sto\sits\sown\scaller,\sexplaining\sthat\sthe\r\nreference\scount\shas\sbeen\sincremented.\s\sCommit\s[510663a99e3a096b],\swhich\ssebres\r\nis\scomplaining\sabout,\sis\sactually\sthe\scommit\sthat\sfixed\sthis\stype\sof\sissue,\sbut\r\nsebres\sdoesn't\sseem\sto\sbe\saware\sof\sthat.\s\s</blockquote>\r\n\r\n"this\sbehavior\sis\sundocumented,"\r\n\r\n<blockquote>\r\nThis\swas\snot\sfalse\sat\sthe\stime\sit\swas\swritten,\sbut\sin\sgeneral\sdevelopers\sshould\sknow\sto\sincrement\sthe\sreference\scount\sof\sa\sTcl_Obj\sbefore\spassing\sit\sto\sa\sfunction\sthey\sdon't\scontrol.\r\n</blockquote>\r\n\r\n"You\sdon't\sknow\swhat\sthe\sobject\s"objPtr"\sis\s(it\sis\snot\syours,\syou'll\sget\sit\sfrom\sforeign\scode\sand\sfrom\sother\speople),\r\nso\sit\scan\shave\srefCount\s==\s0,\s1\sand\slarger\sas\s1.\r\nHow\syou\scan\sset\sit\sto\sthe\svariable\svarName,\sinside\sthis\sfunction\sand\swhat\syou'll\sdo\sin\serror\scase.\r\nIf\syou'll\sincrement\sit,\spossibly\syou\scan\s(unwanted)\sdestroy\sit\sin\serror\scase\sby\sthe\spaired\sdecrement.\r\nIf\syou\sdon't\sincrement\sit,\syou\scan\snot\sbe\ssure\swhat\sdo\syou\swant\sto\sdo\sin\sthe\s"\r\n\r\n<blockquote>\r\nIf\ssomeone\selse\spasses\syou\sa\sTcl_Obj\swith\sa\srefCount\sof\s0,\sthen\sthey\sare\r\nexpecting\syou\sto\sclean\sit\sup\sif\sneeded,\sso\sincrementing\sit\sand\sdecrementing\sit\r\nis\snot\sa\sproblem.\s\sIf\syou\swant\sit\sto\sbe\sunshared\sin\sorder\sto\sefficiently\smodify\r\nthe\sinternal\srepresentation,\sthen\smodify\sthe\sTcl_Obj\sand\sthen,\sdepending\son\r\nyour\sneeds,\seither\sincrement\sthe\sreference\scount\sor\sdon't\sbefore\spassing\sit\sto\r\nTcl_ObjSetVar2,\swhich\shandles\sthe\sunshared\sobject\scorrectly\sin\sthe\serror\scase.\r\nIt's\sstarting\sto\ssound\slike\sthe\sbug\syou're\scomplaining\sabout\sis\sin\sthe\sdesign\r\nof\syour\sown\ssystem.\r\n</blockquote>\r\n\r\n"Meanwhile\sI've\simplemented\sboth\sthings\sfor\smy\sown\stcl-fork,\sand\sit\sworks\sfine\sand\ssaves\sme\s\r\nmany\stime,\swork\sand\scode-lines\s(not\sto\smention\sthe\sreadability)."\r\n\r\n<blockquote>\r\nYou\sare\swelcome\sto\scommit\shigh-quality\swork.\s\sIf\sit's\sgood,\speople\swill\sbe\sglad\r\nto\shave\sit\sin\sthe\simplementation\so\sTcl.\s\sNo\sneed\sfor\sa\sbug\sreport.\s\sThe\scode\r\nwill\ssuffice.\s\sIf\sit's\snot\sa\sgood\scontribution\sit\swill\sbe\sduly\sreverted.\r\n</blockquote>\r\n\r\n"or\sadditionally\sthe\s"obscure"\shack\swith\sauto-decrement\sin\sthe\serror\scase\swill\sremain"\r\n\r\n<blockquote>\r\nIt's\snot\san\sobscure\shack.\s\sIt's\ssimply\sthe\sequivalent\sof\sincrementing\sthe\r\nreference\scount\sand\sthen\sdecrementing\sit\sin\sthe\serror\scase.\r\n</blockquote>\r\n\r\n\r\nThe\smost\srecent\scode\spresented\sby\ssebres\sas\san\sexample\sof\sthe\sissue\she\sis\r\nreporting\sis\sactually\sjust\san\sexample\sof\scode\sthat\sis\sitself\sbuggy:\s\r\n\r\n<code><verbatim>\r\nTcl_Obj\s*\sCacheSetObj(Tcl_Obj\s*varName,\sTcl_Obj\s*objPtr)\r\n{\r\n\s\sTcl_ObjSetVar2(interp,\scache,\svarName,\sobjPtr,\s0);\r\n\s\sreturn\sobjPtr;\r\n}\r\n\r\n...\r\n\r\nif\s((objPtr\s=\sCacheGetObj(someName))\s!=\sNULL)\r\n\s\sreturn\sobjPtr;\r\nreturn\sCacheSetObj(someName,\sSomethingToObtainObj(...));\r\n</verbatim></code>\r\n\r\nThe\sexample\sfailed\sto\smake\suse\sof\sthe\sresult\sof\sTcl_ObjSetVar2.\s\sAs\sthe\r\ndocumentation\swarns,\sit\sis\snot\scorrect\sto\ssimply\sreturn\sobjPtr\sbecuause\sthe\r\nTcl_Obj\s*\sreturned\sby\sTcl_ObjSetVar2\smay\sbe\san\sentirely\sdifferent\sobject.\s\sThe\r\ncorrect\scode\swould\slook\slike\sthis:\r\n\r\n<code><verbatim>\r\nTcl_Obj\s*\sCacheSetObj(Tcl_Obj\s*varName,\sTcl_Obj\s*objPtr)\r\n{\r\n\s\sreturn\sTcl_ObjSetVar2(interp,\scache,\svarName,\sobjPtr,\s0);\r\n}\r\n\r\n...\r\n\r\nif\s((objPtr\s=\sCacheGetObj(someName))\s!=\sNULL)\r\n\s\sreturn\sobjPtr;\r\nreturn\sCacheSetObj(someName,\sSomethingToObtainObj(...));\r\n</verbatim></code>\r\n\r\nNotice\sthat\sthere\sis\sno\sreason\sfor\sCacheSetObj\sto\smanipulate\sthe\sreference\r\ncount\sof\sobjPtr\sat\sall,\sand\sthat\sTcl_ObjSetVar2\scorrectly\scleans\sup\sobjPtr\sif\r\nneeded.\r\n\r\nsebres\shas\shad\sover\sfive\syears\sto\scome\sup\swith\ssome\sworking\scode\sthat\r\ndemonstrates\ssome\smemory\sleak,\ssome\ssegmentation\sfault,\sor\ssome\sother\r\nunexpected\sbehaviour\sthat\sjustifies\sthis\sreport,\sand\shas\sso\sfar\sfailed.\s\s\sThis\r\nreport\swill\snow\sbe\sclosed\sas\s"invalid".\s\sMy\sopinion\sis\sthat\ssebres\sis\sentirely\r\nwrong\sabout\sthis\sissue.\s\sThe\smost\slikely\sexplanation\sis\sthat\she\sis\smisusing\sthe\r\nTcl\sAPI\sand\sthen\sblaming\sproblems\son\sTcl.\s\sIf\she\swishes\sto\sreopen\sthis\srepor,\r\nsebres\sshould\ssupply\sa\scomplete\sworking\sexample\sthat\sreproduces\san\sissue.
J login pooryorick
J mimetype text/x-fossil-wiki
K 578155d5a19b348dc1a9fe96cc2c067a59326a89
U pooryorick
Z edaad6989f6e8836e19e781ff657ab00