Closed Bug 1234830 Opened 4 years ago Closed 4 months ago

[CID 1242894][CID 1242852] unused values

Categories

(NSS :: Libraries, defect, P3)

defect

Tracking

(firefox46 affected)

RESOLVED FIXED
Tracking Status
firefox46 --- affected

People

(Reporter: franziskus, Assigned: franziskus)

References

(Blocks 2 open bugs)

Details

(Keywords: coverity, Whiteboard: CID1242894,CID1242852)

Attachments

(2 files, 3 obsolete files)

There are unused values in jarfile.c jar_physical_inflate and JAR_extract.
Attached patch Bug1234830.patch (obsolete) — Splinter Review
clean up some return values.
Attachment #8701465 - Flags: review?(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?
(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)
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)
Attachment #8701465 - Flags: review?(kaie) → review-
Attached patch Bug1234830.patchSplinter Review
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)
(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
Priority: -- → P3
Assignee: franziskuskiefer → nobody
Attached patch 1234830-jar-result.patch (obsolete) — Splinter Review

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

Flags: needinfo?(kaie)
Attached patch 1234830-v4.patch (obsolete) — Splinter Review

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.

Attachment #9075635 - Attachment is obsolete: true
Attachment #9075641 - Flags: review?(franziskuskiefer)
Comment on attachment 9075641 [details] [diff] [review]
1234830-v4.patch

Franziskus won't be back for several months, moving to Kevin
Attachment #9075641 - Flags: review?(franziskuskiefer) → review?(kjacobs.bugzilla)

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

QA Contact: jjones

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.

Flags: needinfo?(kaie)
Attached patch 1234830-v5.patchSplinter Review

add missing goto / cleanup changes

Assignee: nobody → kaie
Attachment #9075641 - Attachment is obsolete: true
Attachment #9075641 - Flags: review?(kjacobs.bugzilla)
Flags: needinfo?(kaie)
Attachment #9092204 - Flags: review?(kjacobs.bugzilla)
Assignee: kaie → franziskuskiefer
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.
Attachment #9092204 - Flags: review?(kjacobs.bugzilla) → review+
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → 3.47
You need to log in before you can comment on or make changes to this bug.