Bug 48401 - CacheIgnoreURLSessionIdentifiers recognizes the wrong key
Summary: CacheIgnoreURLSessionIdentifiers recognizes the wrong key
Status: RESOLVED FIXED
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: mod_cache (show other bugs)
Version: 2.5-HEAD
Hardware: PC Linux
: P2 normal (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
URL:
Keywords: FixedInTrunk, PatchAvailable
Depends on:
Blocks:
 
Reported: 2009-12-16 21:23 UTC by Dodou Wang
Modified: 2012-02-26 17:01 UTC (History)
1 user (show)



Attachments
Fix the cache_generate_key_default's wrong behavior (531 bytes, application/octet-stream)
2009-12-16 21:23 UTC, Dodou Wang
Details
Alternate approach to fix edge case (2.38 KB, patch)
2009-12-17 00:37 UTC, Ruediger Pluem
Details | Diff
For Removing The '&' (3.06 KB, patch)
2009-12-17 03:20 UTC, Dodou Wang
Details | Diff
Remove trailing & (2.75 KB, patch)
2009-12-17 05:37 UTC, Ruediger Pluem
Details | Diff
Remove trailing &, fixed (2.79 KB, patch)
2009-12-17 07:55 UTC, Dodou Wang
Details | Diff
Revised version (2.80 KB, patch)
2009-12-17 10:03 UTC, Ruediger Pluem
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dodou Wang 2009-12-16 21:23:57 UTC
Created attachment 24718 [details]
Fix the cache_generate_key_default's wrong behavior

Suppose the querystring is:     PRE_key1=dodou&key2=wang
and the httpd.conf contains:    CacheIgnoreURLSessionIdentifiers key1

Then the cache_generate_key_default function generates the wrong cachekey, it delete the "key1=dodou&", then the cackekey turns to be : "http://xxx.com/main.php?PRE_key2=wang", which is wrong behavior.

The attachment is a patch to fix it.


Thand you
Dodou Wang
Comment 1 Ruediger Pluem 2009-12-17 00:35:10 UTC
Thanks for the patch, but I guess it misses an additional edge case. Lets use your configuration with a slightly different querystring:

Suppose the querystring is:     PRE_key1=dodou&key2=wang&key1=ignore_me
and the httpd.conf contains:    CacheIgnoreURLSessionIdentifiers key1

With the old code we would end up with

PRE_key2=wang&key1=ignore_me

With your patch we would end up with

PRE_key1=dodou&key2=wang&key1=ignore_me

but the correct result should be

PRE_key1=dodou&key2=wang

Can you please crosscheck if the patch I attach in minute also fixes your problem?
Comment 2 Ruediger Pluem 2009-12-17 00:37:35 UTC
Created attachment 24720 [details]
Alternate approach to fix edge case
Comment 3 Dodou Wang 2009-12-17 03:20:54 UTC
Created attachment 24722 [details]
For Removing The '&'

(In reply to comment #1)
> Thanks for the patch, but I guess it misses an additional edge case. Lets use
> your configuration with a slightly different querystring:
> 
> Suppose the querystring is:     PRE_key1=dodou&key2=wang&key1=ignore_me
> and the httpd.conf contains:    CacheIgnoreURLSessionIdentifiers key1
> 
> With the old code we would end up with
> 
> PRE_key2=wang&key1=ignore_me
> 
> With your patch we would end up with
> 
> PRE_key1=dodou&key2=wang&key1=ignore_me
> 
> but the correct result should be
> 
> PRE_key1=dodou&key2=wang
> 
> Can you please crosscheck if the patch I attach in minute also fixes your
> problem?

Thank you for the review. 
Your patch did resolve the case above, but I found another problem:

Suppose the querystring is:     PRE_key1=dodou&key2=wang&key1=ignore_me
and the httpd.conf contains:    CacheIgnoreURLSessionIdentifiers key1

with your patch , the result is:   PRE_key1=dodou&key2=wang&
There is a "&" in the end, it is not wanted. 
consider the two querystring: 
first:  PRE_key1=dodou&key2=wang&key1=ignore_me
second: PRE_key1=dodou&key2=wang

both your patch and the old code will treat them as different keys, just because of the '&'.

so i changed a little of your patch to fix it, can you please check whether it works all right?

Thank you very much:)
Dodou Wang
Comment 4 Ruediger Pluem 2009-12-17 05:36:50 UTC
Thanks for the pointer. I solved it a little different. Care to test?
Comment 5 Ruediger Pluem 2009-12-17 05:37:31 UTC
Created attachment 24723 [details]
Remove trailing &
Comment 6 Dodou Wang 2009-12-17 07:55:03 UTC
Created attachment 24727 [details]
Remove trailing &, fixed

(In reply to comment #4)
> Thanks for the pointer. I solved it a little different. Care to test?

I am afraid that your patch has some risk. 

if the querystring starts with the identifier, and the idenitfier is the only one param of the querystring, then the follow codes in your patch will be executed.
1.  querystring = ""
2.  qs_len_idx = strlen(querystring) - 1
3.  if (querystring[qs_len_idx] == '&') ...

it will compare querystring[-1] with '&', the querystring is allocate from the pool, so the querystring[-1] maybe any strange value, including the '&'. The memory may be corrupted at that time.

My previous patch 24722 did not have this risk. 
Or, if we want to fix it in your style, I think this attachment works.


Thanks
Dodou Wang
Comment 7 Ruediger Pluem 2009-12-17 10:03:55 UTC
Created attachment 24729 [details]
Revised version

Thanks for pointing out, but I guess your patch dosen't handle the following case:

PRE_key1=a&key2=b&key1=c&key3=blah

So I made a new one that should also handle this case :-)
Comment 8 Dodou Wang 2009-12-17 14:24:01 UTC
(In reply to comment #7)
> Created an attachment (id=24729) [details]
> Revised version
> 
> Thanks for pointing out, but I guess your patch dosen't handle the following
> case:
> 
> PRE_key1=a&key2=b&key1=c&key3=blah
> 
> So I made a new one that should also handle this case :-)

er... have you tried the case above? I tried the case, and didn't find anything wrong in my previous patch, can you explain your guess?

Maybe you means that in some cases, the querystring won't be ended by '&'?
But if the querystring isn't "", then it must be generated by cutting off the identifier, the follow codes guarantees that the end of it must be '&'.

else {
    char *complete;

    /*
     * In order to avoid subkey matching (PR 48401) prepend
     * identifier with a '&' and append a '='
     */
    complete = apr_pstrcat(p, "&", *identifier, "=", NULL);
    param = strstr(querystring, complete);
    /* If we found something we are sitting on the '&' */    
    if (param) {
        param++;                                             
    }
}                                                            
if (param) {
    char *amp;

    if (querystring != param) {                              
        querystring = apr_pstrndup(p, querystring,
                               param - querystring);        


So in my opinion, we can be confident to change the last character to '\0', as in my patch 24727. the comparaion between '&' in your patch maybe not needed any more, isn't it?

Thanks:)
Dodou Wang
Comment 9 Ruediger Pluem 2009-12-18 08:05:26 UTC
You are correct. I somehow made a mistake when going through your patch. Committed it as r892289 with a slight modification of the comment to make it a little bit clearer why the last char in this case must be an '&'.
Comment 10 Christophe JAILLET 2011-06-29 21:18:45 UTC
If the patch as been committed, this bug report should be closed, no ?
Comment 11 Stefan Fritsch 2012-02-26 17:01:40 UTC
fixed in 2.4.1