Skip to content

Spelling#10743

Merged
jketema merged 41 commits intogithub:mainfrom
jsoref:spelling
Oct 12, 2022
Merged

Spelling#10743
jketema merged 41 commits intogithub:mainfrom
jsoref:spelling

Conversation

@jsoref
Copy link
Copy Markdown
Contributor

@jsoref jsoref commented Oct 9, 2022

This is not a full run of check-spelling. It's mostly limited to .qhelp files -- The action's sarif support is very new; codeql doesn't like >5000 item reports, and I hadn't written the code to automatically trim (I've drafted some thoughts but won't be able to implement them this week).

This PR corrects misspellings identified by the check-spelling action.

The misspellings have been reported at https://github.com/jsoref/codeql/actions/runs/3214090382#summary-8787786982

The action reports that the changes in this PR would make it happy: jsoref@30e5caf

Note: this PR does not include the action. If you're interested in running a spell check on every PR and push, that can be offered separately.

Copy link
Copy Markdown
Contributor Author

@jsoref jsoref left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some items were automatically suggested by Google Sheets (and many more could have been had I fed it the input sooner).

All fault mine.

</ol>

<p>This rule finds magic numbers for which there is no pre-existing named
<p>This rule finds magic numbers for which there is no preexisting named
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to drop any particular change(s). This is a word, and thus there's no particular reason to use the hyphenated flavor.

</overview>
<recommendation>
<p>Check that the unused static variable does not indicate a defect, for example, an unhandled case. If the static variable is genuinuely not needed,
<p>Check that the unused static variable does not indicate a defect, for example, an unhandled case. If the static variable is genuinely not needed,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming this (and similar items) is (are) user facing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, .qhelp files are used to generate HTML, for example you can see this mistake here: https://codeql.github.com/codeql-query-help/cpp/cpp-unused-static-variable/

<example>
<p>In the example below, the <code>sockfd</code> socket may remain open if an error is triggered.
The code should be updated to ensure that the socket is always closed when when the function ends.
The code should be updated to ensure that the socket is always closed when the function ends.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes there's a reason for doubled words. There didn't seem to be one here.

Comment on lines 9 to 12

<p>Due to the nature of logical operation result value, only the lowest bit could possibly be set, and it is unlikely to be intent in bitwise opeartions. Violations are often indicative of a typo, using a logical-not (<code>!</code>) opeartor instead of the bit-wise not (<code>~</code>) operator. </p>
<p>Due to the nature of logical operation result value, only the lowest bit could possibly be set, and it is unlikely to be intent in bitwise operations. Violations are often indicative of a typo, using a logical-not (<code>!</code>) operator instead of the bit-wise not (<code>~</code>) operator. </p>
<p>This rule is restricted to analyze bit-wise and (<code>&amp;</code>) and bit-wise or (<code>|</code>) operation in order to provide better precision.</p>
<p>This rule ignores instances where a double negation (<code>!!</code>) is explicitly used as the opeartor of the bitwise operation, as this is a commonly used as a mechanism to normalize an integer value to either 1 or 0.</p>
<p>This rule ignores instances where a double negation (<code>!!</code>) is explicitly used as the operator of the bitwise operation, as this is a commonly used as a mechanism to normalize an integer value to either 1 or 0.</p>
<p>NOTE: It is not recommended to use this rule in kernel code or older C code as it will likely find several false positive instances.</p>
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gave me a headache, since the file is in a directory named Likely Typos and thus my initial response was "oops, I better ignore this file". But, this doesn't appear to be the likely typos it was talking about.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for digging into this.

<references>

<li>C# Corner, <a href="http://www.c-sharpcorner.com/UploadFile/rmcochran/csharp_interrfaces03052006095933AM/csharp_interrfaces.aspx">C# Interface Based Development</a>.</li>
<li>C# Corner, <a href="https://www.c-sharpcorner.com/article/C-Sharp-interface-based-development/">C# Interface Based Development</a>.</li>
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just me happily following a redirect which coincidentally fixes the typo check-spelling complained about. As a bonus, it changes an http url to an https url.

<recommendation>
<p>
In the <code>onReceive</code> method of a <code>BroadcastReciever</code>, the action of the received Intent should be checked. The following code demonstrates this.
In the <code>onReceive</code> method of a <code>BroadcastReceiver</code>, the action of the received Intent should be checked. The following code demonstrates this.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes the document agree w/ line 9...

<p>The second example shows how a JMX Server is initialized securely if the <code>RMIConnectorServer</code> class is used.</p>

<sample src="CorrectRMIConnectorServerEnvironmentInitalisation.java" />
<sample src="CorrectRMIConnectorServerEnvironmentInitialisation.java" />
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is renamed above

Comment thread python/ql/lib/semmle/python/web/flask/General.qll Outdated
| test.py:22:10:22:24 | ControlFlowNode for Attribute() | test.py:21:11:21:18 | ControlFlowNode for source() | test.py:22:10:22:24 | ControlFlowNode for Attribute() | test flow (naive): test_simple |
| test.py:33:10:33:12 | ControlFlowNode for val | test.py:29:11:29:18 | ControlFlowNode for source() | test.py:33:10:33:12 | ControlFlowNode for val | test flow (naive): test_alias |
| test.py:41:10:41:12 | ControlFlowNode for val | test.py:45:11:45:18 | ControlFlowNode for source() | test.py:41:10:41:12 | ControlFlowNode for val | test flow (naive): test_accross_functions |
| test.py:41:10:41:12 | ControlFlowNode for val | test.py:45:11:45:18 | ControlFlowNode for source() | test.py:41:10:41:12 | ControlFlowNode for val | test flow (naive): test_across_functions |
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not particularly certain about this change... If it's a problem, I'll drop it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. As long as both the test input and output reflect the typo fix, then everything should continue to work just fine.



def test_accross_functions():
def test_across_functions():
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the pieces should be changed, so it seems reasonable to me

@jsoref
Copy link
Copy Markdown
Contributor Author

jsoref commented Oct 9, 2022

Oh, I should note that check-spelling-sandbox#1 has roughly what this looks like as sarif output. Sadly, I don't think you can see much w/o being a member of the repository. Which means I need to find someone to talk to about how this stuff works. I was asked about the possibility of changing check-spelling to use sarif as its primary output mechanism, but if most people can't see its reports, that'd make it fairly painful. I'm very new to github's sarif handling and could use a hand understanding it, so if someone has time to talk... I'm on GitHub Security Lab Slack...

Copy link
Copy Markdown
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CPP looks good to me, with one point to discuss.

Comment thread cpp/ql/src/Best Practices/Magic Constants/MagicConstantsString.qhelp Outdated
@aschackmull
Copy link
Copy Markdown
Contributor

Thanks for submitting this PR!
Generally looks good to me, although I suspect that the changes in python/ql/lib/semmle/python/web/flask/General.qll and python/ql/lib/semmle/python/web/flask/Response.qll ought to be reverted as it otherwise might be a breaking change (and since it's deprecated, it's bound to go away at some point anyway).

@calumgrant calumgrant requested a review from tausbn October 10, 2022 09:09
Copy link
Copy Markdown
Contributor

@tausbn tausbn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! I have made a few comments and suggestions.

Comment thread python/ql/lib/semmle/python/web/flask/General.qll Outdated
| test.py:22:10:22:24 | ControlFlowNode for Attribute() | test.py:21:11:21:18 | ControlFlowNode for source() | test.py:22:10:22:24 | ControlFlowNode for Attribute() | test flow (naive): test_simple |
| test.py:33:10:33:12 | ControlFlowNode for val | test.py:29:11:29:18 | ControlFlowNode for source() | test.py:33:10:33:12 | ControlFlowNode for val | test flow (naive): test_alias |
| test.py:41:10:41:12 | ControlFlowNode for val | test.py:45:11:45:18 | ControlFlowNode for source() | test.py:41:10:41:12 | ControlFlowNode for val | test flow (naive): test_accross_functions |
| test.py:41:10:41:12 | ControlFlowNode for val | test.py:45:11:45:18 | ControlFlowNode for source() | test.py:41:10:41:12 | ControlFlowNode for val | test flow (naive): test_across_functions |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. As long as both the test input and output reflect the typo fix, then everything should continue to work just fine.

smowton
smowton previously approved these changes Oct 10, 2022
Copy link
Copy Markdown
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go LGTM

hvitved
hvitved previously approved these changes Oct 10, 2022
Copy link
Copy Markdown
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C# 👍 Thanks

aibaars
aibaars previously approved these changes Oct 10, 2022
Copy link
Copy Markdown
Contributor

@aibaars aibaars left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 For the Ruby changes. Thanks!

@aibaars
Copy link
Copy Markdown
Contributor

aibaars commented Oct 10, 2022

Note: we just merged #10753 to fix a bug in the Render QHelp changes CI job.

@jsoref jsoref dismissed stale reviews from aibaars, hvitved, and smowton via b5bc021 October 11, 2022 04:21
@jsoref jsoref requested a review from a team as a code owner October 11, 2022 04:21
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C++ LGTM.

@jketema
Copy link
Copy Markdown
Contributor

jketema commented Oct 12, 2022

This is now a subset of what was approved by everyone and all tests pass, so I'll go ahead and merge this.

@jketema jketema merged commit d389a18 into github:main Oct 12, 2022
@jketema
Copy link
Copy Markdown
Contributor

jketema commented Oct 12, 2022

@jsoref thanks a lot for the fixes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.