1171018 - nsTXTToHTMLConv.cpp: Value stored to 'cursor' is never read

Status: RESOLVED WONTFIX

(Core :: Networking, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

Found by scan-build:
https://people.mozilla.org/~sledru/reports/fx-scan-build/report-nsTXTToHTMLConv.cpp-OnDataAvailable-42-1.html#EndPath

https://dxr.mozilla.org/mozilla-central/source/netwerk/streamconv/converters/nsTXTToHTMLConv.cpp?from=nsTXTToHTMLConv.cpp#174

Comment 1

[Vikram]

Hi Sylvestre

I would like to take this as my first bug. 
The bug description seems self explanatory for the fix.

However, could you guide me as to how I go about testing and submitting my changes ?

Thanks!

Comment 2

[Sylvestre Ledru [:Sylvestre]]

I don't think this requires testing (besides compiling the code).
You should have a look to this doc to create a patch:
https://developer.mozilla.org/docs/Mercurial_FAQ

Comment 3

[sumith1896]

Hi Sylvestre, 

I would love to fix my first bug for mozilla, I am proficient in c++.
Could you tell me how to compile this. The sole file won't compile due to other dependencies.

Comment 4

[sumith1896]

What I understand from the code is the you need not set `cursor=0;` as in the next iteration it is reset to 0, is that a valid fix?
If yes how do I send a patch?

Comment 5

[Valentin Gosu [:valentin] (he/him)]

(In reply to sumith1896 from comment #4)
> What I understand from the code is the you need not set `cursor=0;` as in
> the next iteration it is reset to 0, is that a valid fix?

Because it's not used in the rest of the block, clang-analyzer warns you of that.
Removing that instruction would probably work, with a comment, that its value shouldn't be used from then on.

> If yes how do I send a patch?

A comprehensive guide to contributing is found at:
http://codefirefox.com/

Guide to building Firefox:
https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

Comment 6

[sumith1896]

I'm on it, thanks

Comment 7

[Vikram]

Attachment: Fix for bug 1171018

I have attached the patch which *should have* removed the unused variable cursor from nsTXTToHTMLConv::OnDataAvailable():nsTXTToHTMLConv.cpp.

I am not sure who would be reviewing it so I left the reviewer field blank in the commit message.

Does this patch do the required change or is there something else that needs to be handled?

Comment 8

[Vikram]

Attachment: Bug1171018.patch

Messed up the last patch so resubmitting the patch

Comment 9

[Vikram]

Attachment: Bug1171018.patch

Somehow my changes were not getting shown in the commit. Trying it one more time to make sure I got it. Sorry about the spam

Comment 10

[Vikram]

Attachment: Bug1171018_ver2.patch

Resync'ed the source code to make sure the removal of cursor shows the change

Comment 11

[sumith1896]

(In reply to Vikram from comment #10)
> Created attachment 8616498 [details] [diff] [review]
> Bug1171018_ver2.patch
> 
> Resync'ed the source code to make sure the removal of cursor shows the change

Better add a comment to not use cursor for thereon, suggesting that it has not been reset

Comment 12

[Patrick McManus [:mcmanus]]

Comment on attachment 8616498 [details] [diff] [review]
Bug1171018_ver2.patch

Review of attachment 8616498 [details] [diff] [review]:
-----------------------------------------------------------------

honestly. I'm getting tired of this category of patch.

basically they all follow the same pattern

sPtr -= k;
sLen -= k;

// compiler warns that sLen is unused

someone submits a patch to remove sLen -=45 .. leaving inconsistent string data and creating a footgun in the code.

another instance of this class of bug satisfied clang by adding "sLen;" afterwards. I'm ok with doing it again, but frankly this is coding around a lack of flexibility in the tool. It should support explicit annotations or an external exception/suppression file. The code itself is frankly better as is - the only value is in getting a better report from the analyzer to see real issues. I'd rather see work go into making the tool work better than fixing non problems in gecko.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

(In reply to Patrick McManus [:mcmanus] from comment #12)
> Comment on attachment 8616498 [details] [diff] [review]
> Bug1171018_ver2.patch
> 
> Review of attachment 8616498 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> honestly. I'm getting tired of this category of patch.
Sorry about that. I didn't realize that most of them are most in "your" code.

> basically they all follow the same pattern
> 
> sPtr -= k;
> sLen -= k;
> 
> // compiler warns that sLen is unused
I don't see a problem in removing useless code but you are the reviewer.

For now, scan-build (built on top of clang analyzer) does not allow to ignore a warning.
I will see how to silent these bugs.

Comment 14

[Patrick McManus [:mcmanus]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #13)

> I don't see a problem in removing useless code but you are the reviewer.
> 

its just a maintenance problem. the string structure is now in a half valid state if that code is removed. If someone comes along and modifies the function this is unexpected and they could easily introduce a serious security bug. Its the same reason its a bad idea to do release/free(x) without assigning x to NULL in the middle of a function.. Consistent state is valuable.

removing the code doesn't even improve performance - we know the compiler is recognizing the situation and optimizing away the assignment.

otoh making the list of warnings have a better signal to noise ratio is a valuable thing. I just would prefer we do that in ways that don't change the intentional state of the gecko code. (annotations, suppression files, etc..)

> For now, scan-build (built on top of clang analyzer) does not allow to
> ignore a warning.
> I will see how to silent these bugs.

Comment 15

[richard.eigenmann]

I was lured to this "Bug" at Mozilla Day in Zurich today. Since I recently decided I wanted to learn some C++ I was attracted by the label "good first bug" and this looks like a nice trivial exercise to practice creating a patch, compiling, submitting etc.

Can we close the bug? It isn't really a bug because, as you say, the compiler throws the unnecessary assignment out of the object code?

If we can't make the analyzer ignore the issue, perhaps it would be sufficient to comment the instruction in the source code to help a future developer consider the prior state of the variable before reusing it and add a link to this Bug ticket so that it doesn't resurface?

Comment 16

[Waqar Arshad]

Hi Sylvestre, 

I would like to take this bug as my first fix. Could you please assign it so that i can start working on it?

Thank You

Comment 17

[Joshpal Sahota]

Attachment: Bug1171018.patch

Here I have taken comment 11 into consideration and commented out the the line of code stating that it is never read and has been taken out.

Comment 18

[Waqar Arshad]

Attachment: Bug 1171018.patch

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Sorry, I am going to wontfix this bug because of comment #12.
I have other similar bugs, please drop me an email if you are interested.