httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Graham Leggett <minf...@sharp.fm>
Subject Re: svn commit: r644746 - in /httpd/httpd/trunk: ./ docs/manual/mod/ include/ modules/session/ server/
Date Sat, 05 Apr 2008 15:26:17 GMT
Ruediger Pluem wrote:

>> +AP_DECLARE(int) ap_unescape_all(char *url);
>> +
>> +/**
> 
> Doesn't this require a minor bump?

I think it does do, yes. Fixed.

>> +AP_DECLARE(char *) ap_escape_path_segment_b(char *c, const char *s);
> 
> This is not a very descriptive name. Shouldn't it be better 
> ap_escape_path_segment_buffer?

It was originally buffer, then I changed it because it was too long. 
You're right though, will change.

>> +AP_DECLARE(void) ap_session_get(request_rec * r, session_rec * z, 
>> const char *key, const char **value)
>> +{
>> +    if (!z) {
>> +        ap_session_load(r, &z);
> 
> Not checking the return value can lead to segfaults as z can be invalid.

The real check is to ensure that z is set, the return value can be 
successful but z can still be NULL (which will happen if no session is
configured), so checking the return value won't help.

Fixed.

>> +    /* decode what we have */
>> +    encoded = apr_pstrcat(r->pool, z->encoded, NULL);
> 
> What is the purpose of this? Did you mean apr_pstrdup? And why
> doing a copy at all if you set z->encoded to NULL later anyway?
> 
>> +    pair = apr_strtok(encoded, sep, &last);

It's because of the line above - strtok stomps all over the buffer, the 
copy ensures the buffer really is ours to stomp on. Sure we set it to 
NULL later, but there is no guarantee that someone else doesn't hold a 
pointer to z->encoded somewhere.

>> +    session_dir_conf *new =
>> +    (session_dir_conf *) apr_pcalloc(p, sizeof(session_dir_conf));
>> +
>> +    new->includes = apr_array_make(p, 10, sizeof(const char **));
>> +    new->excludes = apr_array_make(p, 10, sizeof(const char **));
> 
> Not that it really matters, but aren't the elements of the arrays char * 
> instead of char **?

Not sure, I think they weren't but it was late when I wrote that, let me 
double check.

Regards,
Graham
--

Mime
View raw message