When the isapi code pulls the REMOTE_USER header from request that lacks it, it gets translated to "" which results in Tomcat creating an empty Principal object. Here is description of the issue: http://marc.info/?l=tomcat-dev&m=142921251105905&w=2 Essentially GET_SERVER_VARIABLE_VALUE("REMOTE_USER", s->remote_user); macro setting s->remote_user to "" instead of NULL
Created attachment 32678 [details] proposed fix Attached is proposed fix.
I've looked at this and while I agree with the analysis and that the proposed patch would fix the issue, I'm not sure that there isn't a better way to fix this. It seems wrong to do NULL -> "" -> NULL. I think either dup_server_value or jk_pool_strdup need to be modified to return NULL rather than "". However, I am concerned that changing either of those may have unexpected side effects. I think Rainer, Mladen or someone else who knows the code better than I needs to look at this.
Sounds like the ISAPI code should be detecting the "" and replacing it with NULL instead. I've never even looked at that code...
The thing is that ISAPI doesn't return null. You pass a buffer ptr and it fills it. In the case of absent REMOTE_USER, ISAPI sets the buffer[0]=0 and &size=0. So essentially the caller cannot differentiate NULL from "". But they are essentially the same thing. Alternatively the fix can go in Tomcat itself to NOT create CoyotePrincipal upon "" OR null string read from AJP which is also correct. If this was my product and I was controlling it, I'd put a fix in both places. Given the difficulty in getting Tomcat Connector releases out, I would be happy to open a bug against Tomcat and get a fix there which will alleviate the issue.
As I am unfamiliar with ISAPI, I was thinking that we'd just check for strlen=0 and replace "" with NULL and be done with it. I think that's exactly what your patch does. Mark didn't like it because it requires what he describes as NULL -> "" -> NULL which he's right is silly. But it looks like it's ISAPI that is silly and there's nothing we can do about it. I think the question is whether or not GET_SERVER_VARIABLE_VALUE will behave this way for most CGI variables and if it should ALWAYS convert "" to NULL, or if this is just some special case.
(In reply to George Stanchev from comment #4) > The thing is that ISAPI doesn't return null. You pass a buffer ptr and it > fills it. In the case of absent REMOTE_USER, ISAPI sets the buffer[0]=0 and > &size=0. So essentially the caller cannot differentiate NULL from "". But > they are essentially the same thing. > > Alternatively the fix can go in Tomcat itself to NOT create CoyotePrincipal > upon "" OR null string read from AJP which is also correct. > > If this was my product and I was controlling it, I'd put a fix in both > places. Given the difficulty in getting Tomcat Connector releases out, I > would be happy to open a bug against Tomcat and get a fix there which will > alleviate the issue. Hi George, docs for GetServerVariable say, the returned size is the number of bytes inlcuding the terminating '\0' character. So an empty string should return buffer[0] = '\0' and size 1. If you are sure, that an absent REMOTE_USER is handled by returning size 0, I would got for the general checking for size 0 in dup_server_value() and handle size 0 as return value NULL. Any chance you can check that? Also any chance to verify, that the size returned by an existing REMOTE_USER is actually the strlen od the REMOTE_USER + 1 (so including the terminating '\0' as docs say)? I applied the change in r1701497. Slightly related is also r1701496. It would be great if you could also test it. Keeping issue open until fix is confirmed.
I could now test the behavior. Unfortunately GetServerVariable does not return size 0 but instead size 1 in this case. So it really returns an exmpty string and there's no way to make a dstinction between a NULL and an empty string. That means we have to make a distinction based on the semantics of the various variables. I'll keep most of them as empty strings for compatibility and will replace the empty string by NULL for REMOTE_USER. We can let it control by another argument, so it will be easy to switch more variables from empty string to NULL as the need arises.
I implemented the overwriting of the empty string return value with an additional arg in r1702073. Especially REMOTE_USER and AUTH_TYPE will now be NULL instead of empty string. Closing this issue now. Will be part of 1.2.42.
Rainer, thanks for this fix! I apologize for not getting back to you quick but I was out of the office over the Labor Day weekend with no access to email. Let me know if you still need anything tested. Thanks again for getting the fix in!