46467 - _ToLower() does not check for null value, thus crashing xpcom

Status: VERIFIED WORKSFORME

(Core :: XPCOM, defect, P3)

Description

[Sean Su]

in the following function:
  static PRUnichar _ToLower(PRUnichar aChar)

it calls CheckCaseConversion() where an assertion can be triggered, but that the 
error is not passed back to _ToLower().  This is causing a crash in the line 
right after CheckCaseConversion() where it tried to dereference a null pointer.

Comment 1

[Sean Su]

the function is in the file mozilla/xpcom/ds/nsCRT.cpp

Comment 2

[Ray Whitmer]

What is the desired behavior of case conversion, if no case converter could be
loaded, since it has no ability to return an error code?

The code seems to indicate that returning an unconverted character, with no
indication that the method failed, is the right thing.  I don't think it is.
If the logic calls for lower case, and the function is not available, then
to me it seems that the program cannot function.  There is no documentation
that I can see that _ToLower is permitted to return upper case if it isn't in
a proper state to return lower case.

Comment 3

[Sean Su]

I agree that it should not alter the case of the character if the converter 
could not be loaded.  Since _ToLower() is not documented to return error values, 
I would be okay if it just returned the character unchanged.  This would be 
better than the crash :)

Comment 4

[Ray Whitmer]

I need to understand why a crash is not better.

If there are important operations relying on the capability of case conversion 
that will silently malfunction, I think a crash is better than returning the 
original character, which is the wrong answer.  The function is not documented 
as returning the right answer if it feels like it.

To me, it seems like the real problem here is that whatever function relies on 
it shouldn't be invoked when it is not available.

Comment 5

[Ray Whitmer]

I need more information here.  Why is it valid for ToLower to silently fail?  Is 
it not better to fix the problem of the proper component not being loaded yet,
and perhaps make the failure more explicit?

Comment 6

[Sean Su]

The problem was that the file that the case converter is in was missing.  This 
part has been fixed.  I just thought that the crash resulting from this missing 
file was a bug.

It is not correct for ToLower to fail silently.  I was thinking that it would 
be better than a crash since there's not a way to return an error.  I agree that 
a more explicit failure needs to be applied here, but I am not sure as to what 
the proper procedure is.

This is not a priority for me at all.  The crash resulting from this bug has 
been fixed for me, but the crash can still happen for someone else.

Comment 7

[Blake Ross]

spam: Adding crash keyword...

Comment 8

[Ray Whitmer]

This works about the way I would expect it to.  The bug is that it is being 
called in a situation where it cannot give a valid defined result.  I guess an 
alternative would be to return -1.  Is that acceptable?

Comment 9

[leger]

marking verified.