https://bz.apache.org/bugzilla/show_bug.cgi?id=49822
--- Comment #4 from Christopher Schultz <ch...@christopherschultz.net> --- I question the quality of this patch. There are a number of issues that I see: 1. The combining of the URI and query string is completely wrong. Only the query string is actually used. I'm not sure if s->query_string can be NULL (instead of simply empty-string), in which case this will crash the process. 2. There is no ensuring that the "req" "combined" URI+query-string is null-terminated. Out-of-bounds read is possible, given that the client controls the inputs. 3. There is a race-condition between seeding the PRNG (which is a process-wide change) and obtaining a random number from the PRNG. Two threads running at the same moment can step on each other and the random numbers may not be correct. 4. After hashing, why bother initializing a random-number generator and then generating a single random number? You already have a nice "random number" generated: the hash you just computed. Just use the hash directly. Also, why not use a standard hashing algorithm? The one here appears to be the ELF hash algorithm[1] which is, I suppose, standard enough. I looked at libapr and it doesn't look like it has any specific hashing algorithms implemented, unless I am very much mistaken. I suppose ELF hash is as good as any out there. Suggestions are welcome. [1] http://www.sco.com/developers/devspecs/gabi41.pdf pages 94-95, figure 5-12. Also https://en.wikipedia.org/wiki/PJW_hash_function I wonder if the ELF hash algorithm requires some kind of attribution. It's such a short bit of code it can't really be implemented any other way. -- You are receiving this mail because: You are the assignee for the bug. --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
