300115 - nsXPInstallManager::OnDataAvailable may consume more data than expected

Status: RESOLVED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect)

Description

[Darin Fisher]

nsXPInstallManager::OnDataAvailable may consume more data than expected.

The code reads data from the given stream into an 8k stack buffer.  If the
|length| parameter passed to OnDataAvailable is less than 8k and if the stream
contains more than |length| bytes, nsXPInstallManager will end up reading more
than the expected number of bytes from the stream.  In most cases this is not a
major problem for the caller; however, in this case it is a problem for
nsXPInstallManager because it decrements length by the number of bytes read from
the stream, and it does not stop reading until length reaches zero.  Since
length is an unsigned quantity, it is possible that reading more than length
bytes would result in length underflowing.  That in turn would cause
nsXPInstallManager to continue looping, and reading from the stream would likely
fail with an NS_BASE_STREAM_WOULD_BLOCK error, terminating the channel and
resulting in a failed XPI download.

This error probably occurs infrequently as additional data would have to arrive
at just the right time and be inserted into the data pipe at just the right
moment to trigger this bug.  But, the race condition does exist, so I'm sure
this bug must have impacted some user somewhere.

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

This patch ensures that we never read more then |length| on the stream.  We do
this by setting |amt| to be the min of 8k and length.

It also ensures that length remains >=0.

Comment 2

[Darin Fisher]

Comment on attachment 188690 [details] [diff] [review]
patch v.1

sizeof(buffer) is good in place of XPI_ODA_BUFFER_SIZE too, but this definitely
works.	Thanks for the patch!  r=darin

Comment 3

[Daniel Veditz [:dveditz]]

Comment on attachment 188690 [details] [diff] [review]
patch v.1

sr=dveditz

Comment 4

[Asa Dotzler [:asa]]

doug, can you land this and mark the keyword fixed1.8 when it's landed on the
branch? Thanks.

Comment 5

[Doug Turner (:dougt)]

1.8 BRANCH:
Checking in nsXPInstallManager.cpp;
/cvsroot/mozilla/xpinstall/src/nsXPInstallManager.cpp,v  <--  nsXPInstallManager.cpp
new revision: 1.135.2.1; previous revision: 1.135
done

TRUNK:
Checking in nsXPInstallManager.cpp;
/cvsroot/mozilla/xpinstall/src/nsXPInstallManager.cpp,v  <--  nsXPInstallManager.cpp
new revision: 1.136; previous revision: 1.135
done