modcplusplus-devel

[Modcplusplus-devel] (johnksterling) mod_cplusplus/src cpp_request.cpp

[Mod C. C. L. <mod...@so...>]

Mod Cplusplus CVS committal

Author  : johnksterling
Project : mod_cplusplus
Module  : src
Dir     : mod_cplusplus/src



Modified Files:
	cpp_request.cpp 


Log Message:
add some constness correctness :) patch submitted by wil...@th...
===================================================================
RCS file: /cvsroot/modcplusplus/mod_cplusplus/src/cpp_request.cpp,v
retrieving revision 1.5
retrieving revision 1.6
diff -u -3 -r1.5 -r1.6
--- cpp_request.cpp	28 Jun 2004 02:52:32 -0000	1.5
+++ cpp_request.cpp	31 Jul 2004 13:58:07 -0000	1.6
@@ -1,6 +1,9 @@
 #define EXPORT_MODCPP
-
 #include "cpp_request.h"
+#include <string>   // for std::string
+#include <stdio.h>  // for sprintf
+
+using std::string;
 
 ApacheRequestRec::ApacheRequestRec(request_rec *r, ApacheRequestRec *pPrev,
                                    ApacheRequestRec *pNext)
@@ -26,49 +29,72 @@
     delete mServer;
 }
 
-void ApacheRequestRec::dump()
+// translate integer to ascii string
+inline string
+istring(int value, const char* format = "%d")
+{
+    enum { TEMP_STORE = 50 };
+    char temp[TEMP_STORE];
+    sprintf(temp,format,value);
+    return string(temp);
+}
+
+
+// translate non-NULL char* to string, NULL to empty string
+inline string
+mstring(const char *cp)
+{
+    return cp ? cp : string();
+}
+
+string ApacheRequestRec::dump_string() const
 {
-    this->rputs("============ ApacheRequestRec ==============\n");
-    this->rprintf("the_request: %s\n", this->the_request());
-    this->rprintf("content_type: %s\n", this->content_type());
-    this->rprintf("assbackwards: %d\n", this->assbackwards());
-    this->rprintf("proxyreq: %d\n", this->proxyreq());
-    this->rprintf("header_only: %d\n", this->header_only());
-    this->rprintf("protocol: %s\n", this->protocol());
-    this->rprintf("proto_num: %d\n", this->proto_num());
-    this->rprintf("hostname: %s\n", this->hostname());
-    this->rprintf("status_line: %s\n", this->status_line());
-    this->rprintf("status: %d\n", this->status());
-    this->rprintf("method: %s\n", this->method());
-    this->rprintf("method_number: %d\n", this->method_number());
-    this->rprintf("allowed: %d\n", this->allowed());
-    this->rprintf("bytes_sent: %d\n", this->bytes_sent());
-    
-    this->rprintf("args: %s\n", this->args());
-    this->rputs("headers_in: \n");
-    this->dump_table(this->headers_in());
-    this->rputs("headers_out: \n");
-    this->dump_table(this->headers_out());
-    this->rputs("err_headers_out:\n");
-    this->dump_table(this->err_headers_out());
-    this->rputs("subprocess_env: \n");
-    this->dump_table(this->subprocess_env());
-    this->rputs("notes: \n");
-    this->dump_table(this->notes());
-    
-    this->rputs("============ /ApacheRequestRec ==============\n");
+    return string("============ ApacheRequestRec ==============\n")
+
+	+ "the_request: "       +      mstring(this->the_request()) + "\n"
+	+ "content_type: "      +      mstring(this->content_type()) + "\n"
+	+ "assbackwards: "      +      istring(this->assbackwards()) + "\n"
+	+ "proxyreq: "          +      istring(this->proxyreq()) + "\n"
+	+ "header_only: "       +      istring(this->header_only()) + "\n"
+	+ "protocol: "          +      mstring(this->protocol()) + "\n"
+	+ "pro  inum: "         +      istring(this->proto_num()) + "\n"
+	+ "hostname: "          +      mstring(this->hostname()) + "\n"
+	+ "status_line: "       +      mstring(this->status_line()) + "\n"
+	+ "status: "            +      istring(this->status()) + "\n"
+	+ "method: "            +      mstring(this->method()) + "\n"
+	+ "method_number: "     +      istring(this->method_number()) + "\n"
+	+ "allowed: "           +      istring(this->allowed()) + "\n"
+	+ "bytes_sent: "        +      istring(this->bytes_sent()) + "\n"
+	+ "args: "              +      mstring(this->args()) + "\n"
+	+ "headers_in: \n"      + table_string(this->headers_in())
+	+ "headers_out: \n"     + table_string(this->headers_out())
+	+ "err_headers_out: \n" + table_string(this->err_headers_out())
+	+ "subprocess_env: \n"  + table_string(this->subprocess_env())
+	+ "notes: \n"           + table_string(this->notes())
+
+	+ "============ /ApacheRequestRec ==============\n";
+}
+
+void ApacheRequestRec::dump() const
+{
+    this->rputs(dump_string().c_str());
 }
 
-void ApacheRequestRec::dump_table(apr_table_t *pTable)
+string ApacheRequestRec::table_string(const apr_table_t *pTable) const
 {
+    string ret_val;
     apr_table_entry_t *pEntry = (apr_table_entry_t *)
         ((apr_array_header_t *)pTable)->elts;
-    int n = ((apr_array_header_t *)pTable)->nelts;
-    int i = 0;
-    while(i < n) {
-	this->rprintf("    [%u] '%s'='%s'\n", i, pEntry[i].key, pEntry[i].val);
-	i++;
+    for (int i = 0, n = ((apr_array_header_t *)pTable)->nelts; i < n; i++) {
+	ret_val += "    [" +   istring(i) + "] '" + pEntry[i].key + "'='"
+	    + pEntry[i].val + "'\n";
     }
+    return ret_val;
+}
+
+void ApacheRequestRec::dump_table(const apr_table_t *pTable) const
+{
+    rputs(table_string(pTable).c_str());
 }
 
 void *ApacheRequestRec::get_dir_config(module *m)
@@ -78,14 +104,14 @@
 
 int ApacheRequestRec::get_basic_auth_pw(const char **sent_pw)
 {
-    return ap_get_basic_auth_pw(mRequest, sent_pw); 
+    return ap_get_basic_auth_pw(mRequest, sent_pw);
 }
 
-int ApacheRequestRec::rputs(const char *str)
+int ApacheRequestRec::rputs(const char *str) const
 {
     return ap_rputs(str, mRequest);
 }
- 
+
 int ApacheRequestRec::rputc(int c)
 {
     return ap_rputc(c, mRequest);
@@ -101,7 +127,7 @@
     int result;
     va_list vp;
     va_start(vp, fmt);
-    result = ap_vrprintf(mRequest, fmt, vp); 
+    result = ap_vrprintf(mRequest, fmt, vp);
     va_end(vp);
     return result;
 }
@@ -115,6 +141,6 @@
 {
     va_list vp;
     va_start(vp, reset);
-    ap_allow_methods(mRequest, reset, vp); 
+    ap_allow_methods(mRequest, reset, vp);
     va_end(vp);
 }

[Modcplusplus-devel] (johnksterling) mod_cplusplus/src cpp_request.cpp

[Mod C. C. L. <mod...@so...>]

Mod Cplusplus CVS committal

Author  : johnksterling
Project : mod_cplusplus
Module  : src
Dir     : mod_cplusplus/src



Modified Files:
	cpp_request.cpp 


Log Message:
rework 3 things:  1) use snprintf instead of sprintf 2) use result from snprintf to build the string (optimization) 3) make utility methods private instance methods to protect them
===================================================================
RCS file: /cvsroot/modcplusplus/mod_cplusplus/src/cpp_request.cpp,v
retrieving revision 1.6
retrieving revision 1.7
diff -u -3 -r1.6 -r1.7
--- cpp_request.cpp	31 Jul 2004 13:58:07 -0000	1.6
+++ cpp_request.cpp	27 Aug 2004 13:08:59 -0000	1.7
@@ -30,19 +30,17 @@
 }
 
 // translate integer to ascii string
-inline string
-istring(int value, const char* format = "%d")
+string ApacheRequestRec::istring(int value, const char* format) const
 {
     enum { TEMP_STORE = 50 };
     char temp[TEMP_STORE];
-    sprintf(temp,format,value);
-    return string(temp);
+    const int len = snprintf(temp, TEMP_STORE, format, value);
+    return string(temp, len);
 }
 
 
 // translate non-NULL char* to string, NULL to empty string
-inline string
-mstring(const char *cp)
+string ApacheRequestRec::mstring(const char *cp) const
 {
     return cp ? cp : string();
 }

Re: [Modcplusplus-devel] (johnksterling) mod_cplusplus/src cpp_request.cpp

[Jonathan W. <co...@co...>]

Re-sending as I screwed it up ...

On Sat, Jul 31, 2004 at 06:58:07AM -0700, Mod Cplusplus CVS List wrote:

> -void ApacheRequestRec::dump()
> +// translate integer to ascii string
> +inline string
> +istring(int value, const char* format = "%d")
> +{
> +    enum { TEMP_STORE = 50 };
> +    char temp[TEMP_STORE];
> +    sprintf(temp,format,value);
> +    return string(temp);
> +}

Could a stringstream be used here instead?
It is Mod _Cplusplus_ after all.

That avoids using fixed-length buffers and format strings, two potential
security problems.

The function could even be made into a template to support more than
integers, but that might be overkill.

If using sprintf(3) is needed for portability or other reasons, how about
the safer snprintf(3) ?  Or removing the format string argument ?  Does
the (unused) flexibility of supplying a format string warrant the
potential problems caused by passing in a format string that results in
more than 50 chars (buffer overrun) or passing in a string that is
incompatible with the integer argument, e.g. "%s" ?

AFAICT that function isn't in an anonymous namespace, and has external
linkage, so could potentially be called from outside that file. IMHO
that means it should be safe against buffer overflows and format string
injection.

Maybe I'm being too paranoid - but that's the frame of mind I like to
adopt when writing apache module code. Web servers are exposed to every
nasty person on the net and I don't think it hurts to be paranoid.

Finally, the return value of sprintf(3) could be used to speed up
the string ctor:

    const int len = sprintf(temp,format,value);
    return string(temp, len);

regards,

jon

-- 
"The whole problem with the world is that fools and fanatics are always
 so certain of themselves, but wiser people so full of doubts."
	- Bertrand Russell

Re: [Modcplusplus-devel] (johnksterling) mod_cplusplus/src cpp_request.cpp

[John K. S. <jo...@st...>]

Hi Jon -

thanks for reviewing this commit!  It was submitted by a user, and 
obviously I didn't review it very carefully :)  comments inline:

On Aug 4, 2004, at 5:50 AM, Jonathan Wakely wrote:

> Could a stringstream be used here instead?
> It is Mod _Cplusplus_ after all.
> That avoids using fixed-length buffers and format strings, two 
> potential
> security problems.

good point.  I don't mind either way, but we should probably keep up 
with stl features.  I'll put that on the todo list.

> The function could even be made into a template to support more than
> integers, but that might be overkill.

probably overkill, but another valid point.

> If using sprintf(3) is needed for portability or other reasons, how 
> about
> the safer snprintf(3) ?  Or removing the format string argument ?  Does
> the (unused) flexibility of supplying a format string warrant the
> potential problems caused by passing in a format string that results in
> more than 50 chars (buffer overrun) or passing in a string that is
> incompatible with the integer argument, e.g. "%s" ?

Totally, I missed that one when I reviewed the code, I am usually 
religious about snprintf.... but I think I like your suggestion of 
removing the format string argument entirely.  add to the todo.

> AFAICT that function isn't in an anonymous namespace, and has external
> linkage, so could potentially be called from outside that file. IMHO
> that means it should be safe against buffer overflows and format string
> injection.

hmm, interesting point.  i'll look into that....

> Maybe I'm being too paranoid - but that's the frame of mind I like to
> adopt when writing apache module code. Web servers are exposed to every
> nasty person on the net and I don't think it hurts to be paranoid.

yup... lots of nasty people on the net.  I don't think you're being 
paranoid at all.

> Finally, the return value of sprintf(3) could be used to speed up
> the string ctor:
>
>     const int len = sprintf(temp,format,value);
>     return string(temp, len);

Ahh, nice one.

I'll have these on the todo list, but if you feel like sending patches 
over I'll be happy to review and commit (and give you credit, of 
course).

sterling

Re: [Modcplusplus-devel] (johnksterling) mod_cplusplus/src cpp_request.cpp

[Jonathan W. <co...@co...>]

On Wed, Aug 04, 2004 at 10:05:49AM -0400, John K. Sterling wrote:

> I'll have these on the todo list, but if you feel like sending patches 
> over I'll be happy to review and commit (and give you credit, of 
> course).

I keep meaning to have a proper look at mod cplusplus, I've never used
it as all the work I do with apache is stuck on v1.3, so we have our
own local patches to allow modules to be written in C++.

I am subscribed to the list though, so look at the cvs commits now and
then, hence this impromptu review  :)

If I ever get a chance to properly look at it (or even better, use it!)
I'll share any ideas I have.

regards,

jon

-- 
"Break your own rules"

Re: [Modcplusplus-devel] (johnksterling) mod_cplusplus/src cpp_request.cpp

[Jonathan W. <co...@co...>]

Hi again,

Looking at that commit again, I'm not sure the const-correctness fixes
are right.
e.g.
    -    char *the_request() { return mRequest->the_request; }
    +    char *the_request() const { return mRequest->the_request; }

This allows the following to compile, but the assertion will fail:

    void f(ApacheRequestRec const& r)  // N.B. const
    {
        const std::string the_orig_request( r.the_request() );

        r.the_request()[0] = '\0';

        assert( the_request == r.the_request() );
    }

Returning char* (as opposed to const char*) from a const method allows
the caller to modify the underlying string. This might be surprising
considering you access the string through a const reference.

The const qualifier on a member function implies that no observable
property of the object will be affected by calling the member. The
function above shows that is not true for ApacheRequestRec.

Care must be taken when returning pointers from const methods. The
member pointer will be const in the context of a const method, but
the pointee is not const and is modifiable.

It might be better to leave the_request() non-const and add a const
overload that returns const char*, so the underlying request_rec struct
and the strings it contains cannot be modified through a const
ApacheRequestRec. i.e. revert the previous change and add this new member:

    const char *the_request() const { return mRequest->the_request; }

Then again, as I've not used the ApacheRequestRec class I don't know if
I'm talking complete rubbish. It might make perfect sense to allow the
request_rec's data to be modified on a const object.

jon


-- 
"He who joyfully marches to music in rank and file has already earned my
 contempt.  He has been given a large brain by mistake, since for him the
 spinal cord would fully suffice."
	- Albert Einstein

Re: [Modcplusplus-devel] (johnksterling) mod_cplusplus/src cpp_request.cpp

[Jonathan W. <co...@co...>]

On Wed, Aug 04, 2004 at 03:33:21PM +0100, Jonathan Wakely wrote:

> Hi again,
> 
> Looking at that commit again, I'm not sure the const-correctness fixes
> are right.
> e.g.
>     -    char *the_request() { return mRequest->the_request; }
>     +    char *the_request() const { return mRequest->the_request; }
> 
> This allows the following to compile, but the assertion will fail:
> 
>     void f(ApacheRequestRec const& r)  // N.B. const
>     {
>         const std::string the_orig_request( r.the_request() );
> 
>         r.the_request()[0] = '\0';
> 
>         assert( the_request == r.the_request() );
          //      ^^^^^^^^^^^ this should be:
          assert( the_orig_request == r.the_request() );
          //      ^^^^^^^^^^^^^^^^
>     }

Sorry for any confusion.

jon

-- 
"Programming is one of the most difficult branches of applied mathematics;
 the poorer mathematicians had better remain pure mathematicians."
	- Edsger Dijkstra

Re: [Modcplusplus-devel] (johnksterling) mod_cplusplus/src cpp_request.cpp

[John K. S. <jo...@st...>]

Hi Jon -

FYI, I applied changes for your 3 suggestions:

1) change sprintf to snprintf (your other suggestions are good, but 
like you said probably overkill)
2) privatize the string utility methods so they're not exposed symbols
3) optimize string creation using the results of snprintf.

Thanks -

John

On Aug 4, 2004, at 5:50 AM, Jonathan Wakely wrote:

> Re-sending as I screwed it up ...
>
> On Sat, Jul 31, 2004 at 06:58:07AM -0700, Mod Cplusplus CVS List wrote:
>
>> -void ApacheRequestRec::dump()
>> +// translate integer to ascii string
>> +inline string
>> +istring(int value, const char* format = "%d")
>> +{
>> +    enum { TEMP_STORE = 50 };
>> +    char temp[TEMP_STORE];
>> +    sprintf(temp,format,value);
>> +    return string(temp);
>> +}
>
> Could a stringstream be used here instead?
> It is Mod _Cplusplus_ after all.
>
> That avoids using fixed-length buffers and format strings, two 
> potential
> security problems.
>
> The function could even be made into a template to support more than
> integers, but that might be overkill.
>
> If using sprintf(3) is needed for portability or other reasons, how 
> about
> the safer snprintf(3) ?  Or removing the format string argument ?  Does
> the (unused) flexibility of supplying a format string warrant the
> potential problems caused by passing in a format string that results in
> more than 50 chars (buffer overrun) or passing in a string that is
> incompatible with the integer argument, e.g. "%s" ?
>
> AFAICT that function isn't in an anonymous namespace, and has external
> linkage, so could potentially be called from outside that file. IMHO
> that means it should be safe against buffer overflows and format string
> injection.
>
> Maybe I'm being too paranoid - but that's the frame of mind I like to
> adopt when writing apache module code. Web servers are exposed to every
> nasty person on the net and I don't think it hurts to be paranoid.
>
> Finally, the return value of sprintf(3) could be used to speed up
> the string ctor:
>
>     const int len = sprintf(temp,format,value);
>     return string(temp, len);
>
> regards,
>
> jon
>
> -- 
> "The whole problem with the world is that fools and fanatics are always
>  so certain of themselves, but wiser people so full of doubts."
> 	- Bertrand Russell
>
>
> -------------------------------------------------------
> This SF.Net email is sponsored by OSTG. Have you noticed the changes on
> Linux.com, ITManagersJournal and NewsForge in the past few weeks? Now,
> one more big change to announce. We are now OSTG- Open Source 
> Technology
> Group. Come see the changes on the new OSTG site. www.ostg.com
> _______________________________________________
> Modcplusplus-devel mailing list
> Mod...@li...
> https://lists.sourceforge.net/lists/listinfo/modcplusplus-devel
>