Assorted fixes. #29

Merged
merged 6 commits into from Jan 22, 2012

Conversation

Projects
None yet
3 participants
@pames
Contributor

pames commented Dec 30, 2011

See individual commit remarks.

@bnoordhuis

View changes

src/mod_auth_cas.c
- for(i = 0; i < strlen(value); i++) {
- d = value[i];
- if( (d < '0' || d > '9') &&
+ for(sz = 0; sz < strlen(value); sz++) {

This comment has been minimized.

@bnoordhuis

bnoordhuis Dec 31, 2011

Contributor

Cache the result of strlen(value)

@bnoordhuis

bnoordhuis Dec 31, 2011

Contributor

Cache the result of strlen(value)

This comment has been minimized.

@forsetti

forsetti Dec 31, 2011

Contributor

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

@forsetti

forsetti Dec 31, 2011

Contributor

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

This comment has been minimized.

@bnoordhuis

bnoordhuis Dec 31, 2011

Contributor

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.

@bnoordhuis

bnoordhuis Dec 31, 2011

Contributor

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.

@bnoordhuis

View changes

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++) {

This comment has been minimized.

@bnoordhuis

bnoordhuis Dec 31, 2011

Contributor

Cache the result of strlen(charsToEncode)

@bnoordhuis

bnoordhuis Dec 31, 2011

Contributor

Cache the result of strlen(charsToEncode)

This comment has been minimized.

@bnoordhuis

bnoordhuis Dec 31, 2011

Contributor

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]

@bnoordhuis

bnoordhuis Dec 31, 2011

Contributor

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]

@bnoordhuis

View changes

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)));

This comment has been minimized.

@bnoordhuis

bnoordhuis Dec 31, 2011

Contributor

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).

@bnoordhuis

bnoordhuis Dec 31, 2011

Contributor

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).

@bnoordhuis

View changes

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++)

This comment has been minimized.

@bnoordhuis

bnoordhuis Dec 31, 2011

Contributor

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

@bnoordhuis

bnoordhuis Dec 31, 2011

Contributor

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

@pames

This comment has been minimized.

Show comment
Hide comment
@pames

pames Dec 31, 2011

Contributor

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).

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@pames

pames Jan 14, 2012

Contributor

friendly ping?

Contributor

pames commented Jan 14, 2012

friendly ping?

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Jan 14, 2012

Contributor

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

Contributor

bnoordhuis commented Jan 14, 2012

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

@forsetti

This comment has been minimized.

Show comment
Hide comment
@forsetti

forsetti Jan 19, 2012

Contributor

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

Contributor

forsetti commented Jan 19, 2012

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

@ghost ghost assigned forsetti Jan 22, 2012

@forsetti

This comment has been minimized.

Show comment
Hide comment
@forsetti

forsetti Jan 22, 2012

Contributor

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)

Contributor

forsetti commented Jan 22, 2012

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

This comment has been minimized.

Show comment
Hide comment
@pames

pames Jan 22, 2012

Contributor

That would be great.

Contributor

pames commented Jan 22, 2012

That would be great.

@forsetti forsetti merged commit c6c2a2c into apereo:master Jan 22, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment