Closed Bug 826052 Opened 7 years ago Closed 7 years ago

HashStore.cpp(252) : warning C4244: 'argument' : conversion from 'int64_t' to 'uint32_t', possible loss of data

Categories

(Toolkit :: Safe Browsing, defect)

All
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 20

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file)

Build warnings:
{
e:/builds/moz2_slave/m-cen-w32-ntly/build/toolkit/components/url-classifier/HashStore.cpp(252) : warning C4244: 'argument' : conversion from 'int64_t' to 'uint32_t', possible loss of data

e:/builds/moz2_slave/m-cen-w32-ntly/build/toolkit/components/url-classifier/HashStore.cpp(327) : warning C4244: 'argument' : conversion from 'int64_t' to 'uint32_t', possible loss of data
}

Now that bug 824247 has made FAIL_ON_WARNINGS affect Windows builds, this blocks bug 825941.
It looks like our file-seeking & file-hashing functions here only take 32-bit unsigned ints, and we're passing them a 64-bit int because that's what our "get file size" function returns.

I think we should just check whether the file size is out of bounds for a uint32_t -- if it is, we should explicitly fail; if it's not, we can static_cast it to a uint32_t for warning-free usage.
QA Contact: dholbert
Assignee: nobody → dholbert
QA Contact: dholbert
Attached patch fix v1Splinter Review
This patch
 (a) adds an early check (and failure case) for file-size being larger than UINT32_MAX

 (b) removes a redundant file-size lookup from CheckChecksum(), and just makes that function take the file-size as a uint32_t argument instead

 (c) Adds some "is subtraction about to wrap below 0?" checks (and NS_WARNINGs / failure cases) since now the file size is unsigned & going below 0 would wrap around to a large number)
Attachment #697203 - Flags: review?(gpascutto)
To be clear, the two chunks of code that are triggering the warnings in
comment 0 are:

> 226 nsresult
> 227 HashStore::Open()
> 228 {
[...]
> 247   int64_t fileSize;
> 248   rv = storeFile->GetFileSize(&fileSize);
> 249   NS_ENSURE_SUCCESS(rv, rv);
> 250 
> 251   rv = NS_NewBufferedInputStream(getter_AddRefs(mInputStream), origStream,
> 252                                  fileSize);

...and...

> 302 nsresult
> 303 HashStore::CalculateChecksum(nsAutoCString& aChecksum,
> 304                              int64_t aSize,
> 305                              bool aChecksumPresent)
> 306 {
[...]
> 326     // Hash everything but last checksum bytes
> 327     rv = hash->UpdateFromStream(mInputStream, aSize-CHECKSUM_SIZE);

NS_NewBufferedInputStream and UpdateFromStream both take uint32_t:
https://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#1040
https://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsICryptoHash.idl#85
Comment on attachment 697203 [details] [diff] [review]
fix v1

Review of attachment 697203 [details] [diff] [review]:
-----------------------------------------------------------------

We should probably return the more specific NS_ERROR_FILE_CORRUPTED in some places, but none of the current code is doing that so let's do that cleanup another time.
Attachment #697203 - Flags: review?(gpascutto) → review+
https://hg.mozilla.org/mozilla-central/rev/10ed2d7d7368
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.