82097 - bidi- arabic- potential buffer overrun in nsBidiUtilsImp::Conv_06_FE_WithReverse

Status: RESOLVED WONTFIX

(Core :: Layout: Text and Fonts, defect)

Description

[Frank Tang]

This code is currently turn off by #ifdef FULL_ARABIC_SHAPING because the code 
is not fully reviewed and have a lot of ARB problems.

we should treat this as individual cases from other shaping routines because the 
priority is different.

It is currently called by two places-
nsCopySupport::HTMLCopy 
and nsFormFrame::UnicodeToNewBytes

Comment 1

[Frank Tang]

This routine have more problems than the others.
1. 
523       PRUnichar buf[8192];
524       PRUint32 len=8192;
525       //reverse the buffer for shaping
526 
527       for(i=beginArabic; i<=endArabic; i++) {
528         buf[i-beginArabic]=aSrcUnichars[endArabic-i+beginArabic];
529       }
530       for(i=0; i<=endArabic-beginArabic; i++) {
531         aSrcUnichars[i+beginArabic]=buf[i];
532       }

use a 8192 array. It have the following problem
a. it use 16K on the stack, which is bad
b. there are no code guarantee i-beginArabic < 8192 in line 528
and there are no code guarantee i < 8192 in line 531

if i-beginArabic >= 8192, then we will ABR
if i >= 8192, then we will ABR
Make the buffer bigger didn't address the issue. It simply cause the bug harder 
to get reproduced and therefore harder to be fixed. 

same issue with 
548           PRUnichar numbuf[20];
549           for(i=beginNumeral; i<=endNumeral; i++){
550             numbuf[i-beginNumeral]=buf[endNumeral-i+beginNumeral];
551           }
552           for(i=0;i<=endNumeral-beginNumeral;i++){
553             buf[i+beginNumeral]=numbuf[i];
554           }

same problem, why it is 20?
use a fixed size in stack is bad
there are no code to guarantee i-beginNumeral < 20 in line 550 and there are no 
code to guarantee i < 20 in line 553

Comment 2

[Maha Abou El-Rous]

depends on the ICU shaping engine drop 82383

Comment 3

[Zach Lipton [:zach]]

Mass-move all BiDi Hebrew and Arabic qa to me, [e-mail]. 
Thank you Gilad for your service to this component, and best of luck to you 
in the future.

Sholom. 

Comment 4

[Katsuhiko Momoi]

QA to mahar.

Comment 5

[Simon Montagu :smontagu]

This has been superseded by the checkin of bug 92797. Not marking FIXED in this
case, since I don't see that the issues raised here have been resolved.

Comment 6

[Simon Montagu :smontagu]

Changing summary (since this has been turned on anyway)

Comment 7

[Maha Abou El-Rous]

Attachment: Malloc instead fixed length buffer

Comment 8

[Ahmad A. Abu-Taha]

Kindly be informed that Ahmad A. Abu-Taha ([e-mail]) from IBM Egypt is 
replacing Maha Abou El-Rous ([e-mail]) in monitoring and receiving 
notifications of Mozilla bugs regarding Arabic.

Comment 9

[chris hofmann]

anyone know if the patch is still good?  mats, can you take a look?

Comment 10

[Simon Montagu :smontagu]

Fixed by removing the code in bug 545059