From http://www.viva64.com/en/a/0078/ Example 2. Invalid verification for file reading operation int PatchFile::LoadSourceFile(FILE* ofile) { ... size_t c = fread(rb, 1, r, ofile); if (c < 0) { LOG(("LoadSourceFile: " "error reading destination file: " LOG_S "\n", mFile)); return READ_ERROR; } ... } PVS-Studio diagnostic message: V547 Expression 'c < 0' is always false. Unsigned type value is never < 0. updater.cpp 1179 This is an example when the code of error handling was written with the "just letting it to be" approach. The programmer didn't even bother to think over what he/she had written and how it would work. Such a verification is an incorrect one: the fread() function uses an unsigned type to return the number of bytes read. This is the function's prototype: size_t fread( void *buffer, size_t size, size_t count, FILE *stream ); The 'c' variable having the size_t type is naturally used to store the result. Consequently, the result of the (c < 0) check is always false. This is a good example. It seems at the first glance that there is some checking here but we find out that it's absolutely useless. The same error can be found in other places as well: V547 Expression 'c < 0' is always false. Unsigned type value is never < 0. updater.cpp 2373 V547 Expression 'c < 0' is always false. Unsigned type value is never < 0. bspatch.cpp 107

Looks like this was introduced in bug 458950.

dolske, thanks for catching that and filing the bug!

Looks like we should be checking to make sure that |c != 1 * r| where |1| is the number of bytes for each element.

I'm not sure if I like the |bytes_per_element| variable name, but it should be more clear to the reader than the current practice of single letter variable names. We also could just embed an assumption in the code that the bytes per element will always be 1, and thus make the check |c != r|. I prefer the explicit check, but am open to feedback.

Comment on attachment 581916 [details] [diff] [review] Patch for bug 710968 Review of attachment 581916 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch, just some minor changes needed. ::: toolkit/mozapps/update/updater/bspatch.cpp @@ +105,5 @@ > while (r) { > + const size_t bytes_per_element = 1; > + const size_t count = (r > SSIZE_MAX) ? SSIZE_MAX : r; > + size_t c = fread(wb, bytes_per_element, count, patchFile); > + if (c != bytes_per_element * count) { Although this isn't currently a bug it is subject to future bugs. If someone changes the value of bytes_per_element it would fail. fread returns the number of elements read and not the number of bytes read. Perhaps you were looking at this documentation (http://www.cplusplus.com/reference/clibrary/cstdio/fread/). It is wrong, and I emailed them to fix it. The correct handling is the number of elements not the number of bytes. So the correct check should be: > if (c != count) { Also to be consistent with the rest of the file please remove the constant bytes_per_element and just use 1 directly instead. ::: toolkit/mozapps/update/updater/updater.cpp @@ +1176,5 @@ > unsigned char *rb = buf; > while (r) { > + const size_t bytes_per_element = 1; > + size_t c = fread(rb, bytes_per_element, r, ofile); > + if (c != bytes_per_element * r) { Ditto the previous comments. @@ +2371,5 @@ > char *rb = mbuf; > while (r) { > + const size_t bytes_per_element = 1; > + size_t c = fread(rb, bytes_per_element, mmin(SSIZE_MAX, r), mfile); > + if (c != bytes_per_element * mmin(SSIZE_MAX, r)) { Ditto the previous comments.

(In reply to Brian R. Bondy [:bbondy] from comment #5) > Perhaps you were looking at this documentation > (http://www.cplusplus.com/reference/clibrary/cstdio/fread/). It is wrong, > and I emailed them to fix it. The correct handling is the number of > elements not the number of bytes. Yeah, that's the documentation I was looking at.

Unfortunately it's the first result that comes up when you search fread. I think I've emailed them before as well.

Thanks this looks correct to me, I'm going to push this to elm and do an auto update just to make sure everything works (which it will). But since this is update code I just want to be extra safe. Then I'll r+.

Thanks :)

Comment on attachment 586781 [details] [diff] [review] Patch for bug 710968 v1.1 Review of attachment 586781 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/update/updater/bspatch.cpp @@ +112,5 @@ > > r -= c; > > if (c == 0 && r) { > rv = UNEXPECTED_ERROR; Please see the note on the following file. ::: toolkit/mozapps/update/updater/updater.cpp @@ +1174,5 @@ > > size_t r = header.slen; > unsigned char *rb = buf; > while (r) { > size_t c = fread(rb, 1, r, ofile); There is another old problem relating to fread handling I just noticed, let's please fix it while in this code. This fread is inside a while(r) but fread will only return less than the number of bytes on error or on eof. So this while-loop doesn't make much sense as is. I think we need to add a line before fread like so: const size_t count = mmin(SSIZE_MAX, r); And then change the fread to read in count instead of r. Also the check at the end of the loop is no longer needed and should be removed. @@ +2378,5 @@ > > r -= c; > rb += c; > > if (c == 0 && r) Ditto this check can be removed since it will be caught by c!= count. If r happened to be 0 it would not have done an fread in the first place because the while(r) would have broken out.

By the way this passed upgrades on elm but I'll retry them after the next patch.

Comment on attachment 587120 [details] [diff] [review] Patch for bug 710968 v1.2 Review of attachment 587120 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for your hard work on this. One last fix mentioned on the most recent patch and we should be good to go. ::: toolkit/mozapps/update/updater/updater.cpp @@ +1176,5 @@ > unsigned char *rb = buf; > while (r) { > + const size_t count = mmin(SSIZE_MAX, r); > + size_t c = fread(rb, 1, count, ofile); > + if (c != r) { This one should be c != count.

Also please pop the patch and update m-c I'd like to make sure it can apply cleanly as there were recent landed changes to m-c. I think it'll be ok but just to be sure.

Sorry for missing that last check. I've fixed the line and pulled the latest changes from m-c and the patch applies cleanly.

Comment on attachment 587534 [details] [diff] [review] Patch for bug 710968 v1.3 Review of attachment 587534 [details] [diff] [review]: ----------------------------------------------------------------- I tested this on the elm branch for both a full and incremental update, both worked no problem. I did notice yet another problem though (not your fault). ::: toolkit/mozapps/update/updater/bspatch.cpp @@ +109,5 @@ > rv = READ_ERROR; > goto end; > } > > r -= c; We're missing incrementing wb here by c. Otherwise it just keeps overwriting the same buffer. I suspect we haven't seen this as a bug because the size is always less than SSIZE_MAX.

Now incrementing wb by c.

Comment on attachment 588227 [details] [diff] [review] Patch for bug 710968 v1.4 Review of attachment 588227 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Thanks for doing these fixes. I already pushed it to elm and tried an update as well with the wb change.