[CID 1242894][CID 1242852] unused values

NEW
Unassigned
(NeedInfo from)

Status

NSS
Libraries
P3
normal
2 years ago
3 months ago

People

(Reporter: fkiefer, Unassigned, NeedInfo)

Tracking

(Blocks: 2 bugs, {coverity})

trunk
coverity
Dependency tree / graph

Firefox Tracking Flags

(firefox46 affected)

Details

(Whiteboard: CID1242894,CID1242852)

Attachments

(1 attachment, 1 obsolete attachment)

There are unused values in jarfile.c jar_physical_inflate and JAR_extract.
Created attachment 8701465 [details] [diff] [review]
Bug1234830.patch

clean up some return values.
Attachment #8701465 - Flags: review?(kaie)

Comment 2

2 years ago
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?
(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.
Flags: needinfo?(kaie)

Comment 4

2 years ago
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.
Flags: needinfo?(kaie)

Updated

2 years ago
Attachment #8701465 - Flags: review?(kaie) → review-
Created attachment 8717372 [details] [diff] [review]
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.
Attachment #8701465 - Attachment is obsolete: true
Flags: needinfo?(kaie)

Comment 6

2 years ago
(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?
Flags: needinfo?(kaie)
(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.
Flags: needinfo?(kaie)
Duplicate of this bug: 1254440
Blocks: 1230156

Updated

8 months ago
Priority: -- → P3
Assignee: franziskuskiefer → nobody
You need to log in before you can comment on or make changes to this bug.