Created attachment 278814 [details] [diff] [review] patch v1 There are some functions that must be called with an SSL PRFD, not with a PRFD of another layer in the same stack as an SSL PRFD. Those functions call ssl_GetPrivate to verify that they have been called with an SSL PRFD, and to return the address of the private sslSocket structure for that PRFD. It also stores the address of the PRFD in the sslSocket. ssl_GetPrivate only checks that the PRFD is an SSL PRFD in DEBUG builds, not in optimized builds. So, in optimized builds, if called with the address of an non-SSL PRFD, it will corrupt that layer's private structure by storing the PRFD address in it somewhere. If the caller of the function that calls ssl_GetPrivate has made an error, passing in the wrong PRFD, ssl_GetPrivate will compound the error by corrupting the private area of that PRFD.
Attachment #278814 - Flags: review?(julien.pierre.boogz)
Attachment #278814 - Flags: review?(julien.pierre.boogz) → review+
Comment on attachment 278814 [details] [diff] [review] patch v1 You might also want to check for a non-NULL fd and set an invalid argument error, since you have those other checks. That would match the assertion completely.
Comment on attachment 278814 [details] [diff] [review] patch v1 Wan-Teh, please SR
Attachment #278814 - Flags: superreview?(wtc)
Comment on attachment 278814 [details] [diff] [review] patch v1 r=wtc. Do you want to check if fd == NULL first?
Attachment #278814 - Flags: superreview?(wtc) → superreview+
Questions for Wan-Teh and Julien: All the callers of ssl_GetPrivate are in the same source file. All but one of them are methods in the PRIOMethods table for SSL. Q1) Wan-Teh, can an NSPR IO method ever get called with a NULL first argument (the PRFileDesc *)? Isn't it the case that if the PRFileDesc* was NULL, that NSPR could not have found the IOMethods table in which to find the address of the IOMethod it called? Can we not infer from the successful call to the IOMethod that the PRFileDesc* was non-NULL and valid? The one exception, the one non-IOMethod caller to ssl_GetPrivate is ssl_SetTimeout. ssl_SetTimeout has two callers, both in sslsecur.c, neither of which is a PRIOMethod. It appears to me that the only reason that ssl_SetTimeout is located in sslsock.c rather than in sslsecur.c (with its callers) is that ssl_GetPrivate is a static function in sslsock.c. In another thread, for which there is no bug filed yet (AFAIK), Julien and I now think that ssl_SetTimeout should call ssl_FindSocket rather than ssl_GetPrivate. (The block comment for ssl_GetPrivate seems to confirm this.) So, I propose to change ssl_SetTimeout to call ssl_FindSocket instead of ssl_GetPrivate, and move ssl_SetTimeout from sslsock.c to sslsecur.c, where it can be static. This can be done because ssl_FindSocket is not static, unlike ssl_GetPrivate. Q2) Julien and Wan-Teh, what do you think of that plan? (I will shortly file the other bug about ssl_SetTimeout.)
Once you fix bug 394271, the block comment for ssl_GetPrivate will be true again, so you don't need to fix this bug. Regarding Q1: I looked at the assertions in ssl_GetPrivate and mechanically expected a matching if statement. I didn't know how this function should be used. So please ignore my question about checking for fd == NULL.
Wan-Teh, Re: comment 5, it still wouldn't hurt to check in this patch in case some other code accidentally calls ssl_GetPrivate again from non-IOLayer functions.
I intend to check this in. It's just not my first priority.
Priority: -- → P2
On trunk Checking in sslsock.c; new revision: 1.53; previous revision: 1.52
Don't forget the branch checkin also.
Wanted to see it on trunk first. ssl/sslsock.c; new revision: 18.104.22.168; previous revision: 22.214.171.124
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.