Forum OpenACS Q&A: Re: Help with openacs installing

Posted by Jim Lynch on
"Use of sprintf is original code" Doesn't make it safe. Yes, as I mentioned, I found that too, and without limiting how many bytes it writes, it has the potential of overwriting the buffer it uses. Assume, for example, that someone changes what's written by some sprintf, without knowing the size of the buffer, this has the potential to overwrite its buffer as well, especially if the sprintf arguments cause it to write more. It doesn't make for good code, it makes for the continued ignorance of an existing bug which should be fixed. I'm just trying to make sure that bugs that could cause security issues are removed and fixed, not kept, and being original code is not a reason to keep a problem.

In the case of writing integers to the tcl result, you don't have to concern yourself with either sprintf or snprintf, you can use other tcl api calls to set the result to the integer directly, and this is what is recommended.

I'll help you in any way I can to make sure these bugs no longer plague us. Would you like me to show the calls that involve writing integers to the tcl result?

Posted by Bjoern Kiesbye on
Hello Jim,

you are right, snprintf is the preferred way, i did use that too when i was writing a simple custom gzip_http_post method. With original code I just meant, it's know to work code,and not new code introduced by the patch, which may lead to problems in case the patch is applied.
As well i was uncertain if fixing this is going to be appreciated at all, because development efforts seem to go more into NaviServer rather then Aolserver. Hearing that there is still interest i can make the changes, and would appreciated some guidance.

First Question is there a preferred way to set the interp result, as there are 2 ways Tcl_SetResult/Tcl_SetObjResult and Tcl_AppendResult. Currently the result is set to a single string, to replace it by the new API I would go with.

Example Integer:

sprintf(interp->result, "%d", nread);

is replaced by

Tcl_SetObjResult(interp, Tcl_NewIntObj(nread));

Example String(1):

interp->result = "could not register callback";

is replaced by

Tcl_SetResult(interp, "could not register callback", TCL_STATIC);

Example String(2):

sprintf(interp->result, "%s", Tcl_GetChannelName(chan));

is replaced by

Tcl_SetResult(interp, Tcl_GetChannelName(chan), TCL_VOLATILE);

Thanks Bjoern

Posted by Jim Lynch on
Yes, -and- these API calls should work in tcl-8.5 as well.


Posted by Bjoern Kiesbye on
Hi Jim,

I updated the patch to use the new tcl API, at one point I had to use the Function Tcl_ObjPrintf, which is available in tcl8.5/.6 only, As the function is not available in tcl8.4 I wrapped it in a #if which checks the tcl version, and in case a the tcl version is 8.4 or lower snprintf is used instead.


+ Tcl_SetObjResult(interp, Tcl_ObjPrintf("%lu", peercert == NULL ? 0 : X509_get_version(peercert) + 1));
+ snprintf(&buf, sizeof(buf), "%lu", peercert == NULL ? 0 : X509_get_version(peercert) + 1);
+ Tcl_SetResult(interp, buf, TCL_VOLATILE);