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

VERIFIED WORKSFORME

Status

()

Core
XPCOM
P3
normal
VERIFIED WORKSFORME
18 years ago
17 years ago

People

(Reporter: Sean Su, Assigned: Ray Whitmer)

Tracking

({crash})

Trunk
x86
Windows NT
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

18 years ago
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.
(Reporter)

Comment 1

18 years ago
the function is in the file mozilla/xpcom/ds/nsCRT.cpp
(Assignee)

Comment 2

18 years ago
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.
(Reporter)

Comment 3

18 years ago
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 :)
(Assignee)

Comment 4

18 years ago
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.
(Assignee)

Comment 5

18 years ago
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?
(Reporter)

Comment 6

18 years ago
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

18 years ago
spam: Adding crash keyword...
Keywords: crash
(Assignee)

Comment 8

18 years ago
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?
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → WORKSFORME

Comment 9

17 years ago
marking verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.