710968 - Updater incorrectly checks fread() retval

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Justin Dolske [:Dolske]]

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

Comment 1

[Robert Strong (they/them - no direct email)]

Looks like this was introduced in bug 458950.

Comment 2

[Robert Strong (they/them - no direct email)]

dolske, thanks for catching that and filing the bug!

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 710968

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 5

[Brian R. Bondy [:bbondy]]

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.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 7

[Brian R. Bondy [:bbondy]]

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

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 710968 v1.1

Comment 9

[Brian R. Bondy [:bbondy]]

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+.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Thanks :)

Comment 11

[Brian R. Bondy [:bbondy]]

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.

Comment 12

[Brian R. Bondy [:bbondy]]

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

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 710968 v1.2

Comment 14

[Brian R. Bondy [:bbondy]]

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.

Comment 15

[Brian R. Bondy [:bbondy]]

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.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 710968 v1.3

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 17

[Brian R. Bondy [:bbondy]]

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.

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 710968 v1.4

Now incrementing wb by c.

Comment 19

[Brian R. Bondy [:bbondy]]

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.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/ed19305f89e6

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/ed19305f89e6