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

Collapse
47: Re: Help with openacs installing (response to 46)
Posted by Bjoern Kiesbye on 07/02/14 06:38 PM
Hi Jim,

I think the tcl8.6 API (http://www.tcl.tk/man/tcl8.6/TclLib/SetResult.htm). should be used to manipulate the interp result, too. Even so I used the solution from nspostgres where a reference to the interp's result string is requested, using the API, and assigned to a local variable, which is then passed to sprintf instead of passing interp->result directly. I think the way it is currently done, partly defeats the purpose of the API, because the interp objects internal state is manipulated from outside code. I did use it because it was a 'known to work' solution, which i needed, and was not certain that there isn't a good reason why it is done this way.

The use of sprintf is original code from nsopenssl, I just changed the variable which is passed to it. Besides the manipulation of interp->result (assuming they will be replaced by calls to the API), sprintf is used twice more to create the channelName, and once to write to buf.

Collapse
50: Re: Help with openacs installing (response to 47)
Posted by Jim Lynch on 07/04/14 01:04 AM
"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?

Collapse
51: Re: Help with openacs installing (response to 50)
Posted by Bjoern Kiesbye on 07/07/14 06:58 PM
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

Collapse
52: Re: Help with openacs installing (response to 51)
Posted by Jim Lynch on 07/07/14 10:25 PM
Yes, -and- these API calls should work in tcl-8.5 as well.

-Jim

Collapse
53: Re: Help with openacs installing (response to 52)
Posted by Bjoern Kiesbye on 07/09/14 12:02 AM
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.

Example:

+#if (TCL_MAJOR_VERSION >= 8) && (TCL_MINOR_VERSION > 4 )
+ Tcl_SetObjResult(interp, Tcl_ObjPrintf("%lu", peercert == NULL ? 0 : X509_get_version(peercert) + 1));
+#else
+ snprintf(&buf, sizeof(buf), "%lu", peercert == NULL ? 0 : X509_get_version(peercert) + 1);
+ Tcl_SetResult(interp, buf, TCL_VOLATILE);
+#endif

http://www.clever-devel.com/file/35043/nsopenssl-interp-result-0.1.1.patch

-bjoern