1234830 - [CID 1242894][CID 1242852] unused values

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Franziskus Kiefer [:franziskus]]

There are unused values in jarfile.c jar_physical_inflate and JAR_extract.

Comment 1

[Franziskus Kiefer [:franziskus]]

Attachment: Bug1234830.patch

clean up some return values.

Comment 2

[Kai Engert [:KaiE:]]

It would have been helpful if this bug report mentioned the values that have been reported as unused.

(a)
I don't like "goto".
How does the introduction of goto avoid unused values?
I cannot review that you're fixing the bug, without knowing which values are unused.

(b)
The very first change in your patch looks like a functional change.

if phy->compression==1, the current code still calls the jar_physical_inflate function.
With your change, it no longer will.

Are you certain that your change won't have any side effect?

Comment 3

[Franziskus Kiefer [:franziskus]]

(In reply to Kai Engert (:kaie) from comment #2)
> It would have been helpful if this bug report mentioned the values that have
> been reported as unused.
> 

In the first change it's result, in the other two it's status. All changes are returning the error code when it occurs (plus doing some clean up if necessary). That might change the logic, but there not really another way to do this if we want to do it. The question is whether it makes sense to execute the code after an error occurred or not.

> (a)
> I don't like "goto".
> How does the introduction of goto avoid unused values?
> I cannot review that you're fixing the bug, without knowing which values are
> unused.
> 

status is unused here, which means the return values are not actually used. I also don't like goto, but it seems the way to go in nss if you don't want to rewrite the cleanup for every return.

> (b)
> The very first change in your patch looks like a functional change.
> 
> if phy->compression==1, the current code still calls the
> jar_physical_inflate function.
> With your change, it no longer will.
> 
> Are you certain that your change won't have any side effect?

No. The alternative would be to keep ignoring the error value  or ignore the value returned from jar_physical_inflate. I'm not sure which one is better.

Comment 4

[Kai Engert [:KaiE:]]

Franziskus, thanks for the explanations.

I think we shouldn't risk unknown functional side effects.


>-	    result = JAR_ERR_CORRUPT;
>+	    return JAR_ERR_CORRUPT;

r-

I suggest to remove the result assignment, and instead add a comment 
  /* For historical reasons, jar_physical_inflate will be called for
     unsupported compression method constants, too. */


BTW, function jar_physical_inflate never uses the "method" parameter. I'd be OK with removing that paramater. (I wonder why the compiler doesn't complain about unused function parameters.)


Regarding your other changes and the unused assignment to the status variable:

I can only find one assignment that's never used, and that's the initial "status = 0".

If that's the place that coverity complained about, then I suggest to use an uninitialized "int status;" declaration, which would be a much simpler change, and all your other changes would be unnecessary.

Comment 5

[Franziskus Kiefer [:franziskus]]

Attachment: Bug1234830.patch

ok, added the comment for the first part.

In the second part the unused variable is status in the while loo (line 353) because it gets overridden. I don't actually think that's a big deal, just wanted to make coverity happy.

If you think it's too much goto, I'm also happy just to add the comment and leave the rest as is.

Comment 6

[Kai Engert [:KaiE:]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #5)
> In the second part the unused variable is status in the while loo (line 353)
> because it gets overridden.

Are you referring to this code? :
  status = inflate (&zs, Z_NO_FLUSH);
  if (status != Z_OK && status != Z_STREAM_END) {

The value assigned to status is used in a comparison. Why does coverity complain that it's unused?

Comment 7

[Franziskus Kiefer [:franziskus]]

(In reply to Kai Engert (:kaie) from comment #6)
> (In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #5)
> > In the second part the unused variable is status in the while loo (line 353)
> > because it gets overridden.
> 
> Are you referring to this code? :
>   status = inflate (&zs, Z_NO_FLUSH);
>   if (status != Z_OK && status != Z_STREAM_END) {
> 
> The value assigned to status is used in a comparison. Why does coverity
> complain that it's unused?

sorry for the delay. the variable gets set here:
> status = JAR_ERR_DISK

and is then overridden in the next iteration of the loop without ever being used:
> status = inflate (&zs, Z_NO_FLUSH);

Note that there are two nested loops, so the break only stops the inner loop.

Comment 9

[Kai Engert [:KaiE:]]

Attachment: 1234830-jar-result.patch

You didn't remove the unnecessary assignment to result in JAR_extract, how about this:

Comment 10

[Kai Engert [:KaiE:]]

Attachment: 1234830-v4.patch

By adding the goto inside the loops, you cause the side effect that the cleanup function inflateEnd will no longer get called.

I suggest this patch, which avoids goto from inside the loops. Instead, it will break out of both loops as soon as we get an error.

The cleanup inflateEnd will continue to be called. If there was a error condition prior to inflateEnd, we'll continue to use that status.

Comment 11

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 9075641 [details] [diff] [review]
1234830-v4.patch

Franziskus won't be back for several months, moving to Kevin

Comment 12

[Kevin Jacobs [:kjacobs]]

The patch looks reasonable, however there's no loser label declared, so it will fail to compile as-is.

Comment 13

[Kai Engert [:KaiE:]]

My patch illustrated my suggestion for the the middle part of Franziskus' patch. I had not tested it, and I failed to include the trailing part of Franziskus' patch. I'll set a reminder for myself to attach a complete patch soon.

Comment 14

[Kai Engert [:KaiE:]]

Attachment: 1234830-v5.patch

add missing goto / cleanup changes

Comment 15

[Kevin Jacobs [:kjacobs]]

Comment on attachment 9092204 [details] [diff] [review]
1234830-v5.patch

NOTE: I think this needs to be clang-formatted (the indentation within `if` blocks differs), otherwise looks good.

Comment 16

[Kai Engert [:KaiE:]]

pushed with clang-format fixes applied:
https://hg.mozilla.org/projects/nss/rev/b6d3f5c95aad504f2f735a89f8bd0621ba4db80d