Closed Bug 100888 Opened 23 years ago Closed 23 years ago

Bad signed conversion in plugin streaming code

Categories

(Core Graveyard :: Plug-ins, defect, P2)

Product:
Core Graveyard
Component:
Plug-ins
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.5

People

(Reporter: peterlubczynski-bugs, Assigned: peterlubczynski-bugs)

Details

(Whiteboard: [PDT+])

Attachments

(1 file, 1 obsolete file)

new patch
3.25 KB, patch
serhunt
: review+
attinasi
: superreview+
Details | Diff | Splinter Review 
from bug 88155....

In ns4xPluginInstance::OnDataAvailable, we keep the return value of NPP_Write
calls in an unsigned integer. However, the NP-API spec says it may return a
signed integer. This may cause problems and even crashes with plugins because we
should not continue the stream if the plugin returns -1. Instead, since the -1
gets converted to an unsigned number, we think the plugin can accept the stream
when in fact it wants to kill it.
Attached patch possible patch (obsolete) — — Splinter Review 

    
    

    
  
Andrei, can you review?
Status: NEW → ASSIGNED
Keywords: nsbranch, 
          
            patch, 
          
            review
Priority: -- → P2
Target Milestone: --- → mozilla0.9.5

    
    

    
  
Peter, |amountRead| is also tested for being greater than zero. Should not it be 
signed? In fact, that crash I saw happened on |memmove| which is under 
|if(amountRead > 0)|.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        new patch
        — 
          — Splinter Review
      

    


  

    
  

        Attachment #50197 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 50267 [details] [diff] [review]
new patch

r=av

        Attachment #50267 -
        Flags: review+

    
    

    
  
Comment on attachment 50267 [details] [diff] [review]
new patch

Wait, you are casting from a signed to unsigned in the call to input->Read : I presume there is no worry about exceeding the limits of the signed INT and getting a bogus negative value? If that is so, then sr=attinasi

        Attachment #50267 -
        Flags: superreview+

    
    

    
  
input->Read is not currently known to return values that would make this a problem.

    
    

    
  
check it in - PDT+

    
    

    
  
check it in - PDT+
Whiteboard: PDT+

    
    

    
  
patch in trunk, will land in branch soon
Whiteboard: PDT+ → [PDT+][patch in trunk]

    
  
Keywords: nsbranch → nsbranch+

    
    

    
  
patch checked into branch, marking FIXED
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Whiteboard: [PDT+][patch in trunk] → [PDT+]

    
    

    
  
verif patch is in branch and trunk. (stamping verif)
Status: RESOLVED → VERIFIED

    
  
Product: Core → Core Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: