Skip to content

Assorted fixes. #29

Merged
merged 6 commits into from Jan 22, 2012

3 participants

@pames
Apereo Foundation member
pames commented Dec 30, 2011

See individual commit remarks.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Dec 31, 2011
src/mod_auth_cas.c
@@ -350,9 +351,9 @@
return(apr_psprintf(cmd->pool, "MOD_AUTH_CAS: Invalid CASCacheCleanInterval (%s) specified - must be numeric", value));
break;
case cmd_cookie_domain:
- for(i = 0; i < strlen(value); i++) {
- d = value[i];
- if( (d < '0' || d > '9') &&
+ for(sz = 0; sz < strlen(value); sz++) {
@bnoordhuis
Apereo Foundation member
bnoordhuis added a note Dec 31, 2011

Cache the result of strlen(value)

@forsetti
Apereo Foundation member
forsetti added a note Dec 31, 2011

Are you expecting this to be reevaluated on each loop, or are you just concerned about earlier use in line 296?

@bnoordhuis
Apereo Foundation member
bnoordhuis added a note Dec 31, 2011

The former, gcc doesn't know that the return value is invariant. It's not critical here since this isn't a hot path but it's a general code quality thing - seeing strlen() in a for loop immediately makes alarm bells go off in my head.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis bnoordhuis commented on an outdated diff Dec 31, 2011
src/mod_auth_cas.c
@@ -764,7 +765,7 @@ void setCASCookie(request_rec *r, char *cookieName, char *cookieValue, apr_byte_
if(str == NULL)
return "";
- size = strlen(str) + 1; /* add 1 for terminating NULL */
+ size = strlen(str);
for(i = 0; i < size; i++) {
for(j = 0; j < strlen(charsToEncode); j++) {
@bnoordhuis
Apereo Foundation member
bnoordhuis added a note Dec 31, 2011

Cache the result of strlen(charsToEncode)

@bnoordhuis
Apereo Foundation member
bnoordhuis added a note Dec 31, 2011

Also, there's a bug just below this line. You can't see it in GH's diff view but here's the code:

        for(i = 0; i < size; i++) {                                              
                for(j = 0; j < strlen(charsToEncode); j++) {                     
                        if(str[i] == charsToEncode[j]) {                         
                                /* allocate 2 extra bytes for the escape sequence (' ' -> '%20') */
                                size += 2;                                       
                                break;                                           
                        }                                                        
                }                                                                
        }

Note how size is incremented but is also used in the comparison i < size, it results in out-of-bound reads of str[i]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis bnoordhuis commented on an outdated diff Dec 31, 2011
src/mod_auth_cas.c
@@ -1167,13 +1168,13 @@ apr_byte_t writeCASCacheEntry(request_rec *r, char *name, cas_cache_entry *cache
path = apr_psprintf(r->pool, "%s.%s", c->CASCookiePath, buf);
if((i = apr_file_open(&f, path, APR_FOPEN_CREATE|APR_FOPEN_WRITE|APR_EXCL, APR_FPROT_UREAD|APR_FPROT_UWRITE, r->pool)) != APR_SUCCESS) {
- ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, "MOD_AUTH_CAS: Service Ticket to Cookie map file '%s' could not be created: %s", path, apr_strerror(i, buf, strlen(buf)));
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, "MOD_AUTH_CAS: Service Ticket to Cookie map file could not be created: %s", apr_strerror(i, buf, strlen(buf)));
@bnoordhuis
Apereo Foundation member
bnoordhuis added a note Dec 31, 2011

Using strlen(buf) here only works accidentally and it might not be big enough to contain the error message (it's only 32 bytes big). It's probably best to use a local variable char errbuf[256] and sizeof(errbuf).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis bnoordhuis commented on an outdated diff Dec 31, 2011
tests/mod_auth_cas_test.c
- for (i = 0; i < sizeof(invalid_headers)/sizeof(char *); i++)
- apr_table_add(headers_in, invalid_headers[i], "Value");
+ for (sz = 0; sz < sizeof(invalid_headers)/sizeof(char *); sz++)
@bnoordhuis
Apereo Foundation member
bnoordhuis added a note Dec 31, 2011

I usually use a #define ARRAY_SIZE(a) (sizeof(a) / sizeof((a)[0])) macro for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pames
Apereo Foundation member
pames commented Dec 31, 2011

Sorry, I didn't fast forward before redoing my fixes. PTAL, I think I've addressed all your comments (and thanks for the feedback too - I did this all in a bit of a rush, so the scrutiny is appreciated).

@pames
Apereo Foundation member
pames commented Jan 14, 2012

friendly ping?

@bnoordhuis
Apereo Foundation member

Sorry for the delay, Phil. LGTM at a glance.

@forsetti
Apereo Foundation member

Yep, LGTM too. Cycles are real short lately, I'll test and commit this weekend if no one else can first.

@forsetti forsetti was assigned Jan 22, 2012
@forsetti
Apereo Foundation member

Tests (make check) fail to compile due to CRYPTO_THREADID not being resolved in mod_auth_cas.h. Need to include openssl/crypto.h in mod_auth_cas.h . Trivial change, I can commit as part of this pull if no objection.

diff --git a/src/mod_auth_cas.h b/src/mod_auth_cas.h
index a9f5aed..6903b5d 100644
--- a/src/mod_auth_cas.h
+++ b/src/mod_auth_cas.h
@@ -30,6 +30,7 @@

#define OPENSSL_THREAD_DEFINES
#include <openssl/opensslconf.h>
+#include <openssl/crypto.h>

#include <openssl/opensslv.h>
#if (OPENSSL_VERSION_NUMBER < 0x01000000)

@pames
Apereo Foundation member
pames commented Jan 22, 2012

That would be great.

@forsetti forsetti merged commit c6c2a2c into Jasig:master Jan 22, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.