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

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

Forgot to include a link to the log from comment 0:
 https://tbpl.mozilla.org/php/getParsedLog.php?id=18385224&full=1&branch=mozilla-central

Comment 2

[Daniel Holbert [:dholbert]]

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.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v1

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)

Comment 4

[Daniel Holbert [:dholbert]]

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 5

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 6

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10ed2d7d7368

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/10ed2d7d7368