Address CodeQL warnings #558

nhosoi · 2023-01-30T19:27:18Z

library/network_connections.py

'except' clause does nothing but pass and there is no explanatory comment.

module_utils/network_lsr/argument_validator.py

'except' clause does nothing but pass and there is no explanatory comment.

module_utils/network_lsr/utils.py

Mixing implicit and explicit returns may indicate an error as implicit returns always return None.

tests/unit/test_network_connections.py

Module 'network_lsr.argument_validator' is imported with both 'import' and 'import from'.
assertTrue(a == b) cannot provide an informative message. Using assertEqual(a, b) instead will give more informative messages.

coveralls · 2023-01-30T19:28:16Z

Coverage: 43.379% (+1.0%) from 42.411% when pulling d889d95 on nhosoi:codeql into c40ede8 on linux-system-roles:main.

nhosoi · 2023-01-30T19:30:08Z

Note: There are 4 more issues codeql reported. The first 3 look false positives. But the 4th one may not. Could you please take a look, @liangwen12year, @richm? Thanks!

CODEQL RESULT
[
  {
    "ruleId": "py/import-and-import-from",
    "rule": {
      "id": "py/import-and-import-from",
      "index": 78,
      "toolComponent": {
        "index": 25
      }
    },
    "message": {
      "text": "Module 'unittest' is imported with both 'import' and 'import from'."
    },
    "locations": [
      {
        "physicalLocation": {
          "artifactLocation": {
            "uri": "tests/unit/test_network_connections.py",
            "uriBaseId": "%SRCROOT%",
            "index": 0
          },
          "region": {
            "startLine": 9,
            "endColumn": 16
          }
        }
      }
    ],
    "partialFingerprints": {
      "primaryLocationLineHash": "ca15ff83070a3cc5:1",
      "primaryLocationStartColumnFingerprint": "0"
    }
  },
  {
    "ruleId": "py/overwritten-inherited-attribute",
    "rule": {
      "id": "py/overwritten-inherited-attribute",
      "index": 148,
      "toolComponent": {
        "index": 25
      }
    },
    "message": {
      "text": "Assignment overwrites attribute validate_one_type, which was previously defined in superclass [Cmd](1)."
    },
    "locations": [
      {
        "physicalLocation": {
          "artifactLocation": {
            "uri": "library/network_connections.py",
            "uriBaseId": "%SRCROOT%",
            "index": 1
          },
          "region": {
            "startLine": 2132,
            "startColumn": 9,
            "endColumn": 83
          }
        }
      }
    ],
    "partialFingerprints": {
      "primaryLocationLineHash": "6236d392535173a3:1",
      "primaryLocationStartColumnFingerprint": "0"
    },
    "relatedLocations": [
      {
        "id": 1,
        "physicalLocation": {
          "artifactLocation": {
            "uri": "library/network_connections.py",
            "uriBaseId": "%SRCROOT%",
            "index": 1
          },
          "region": {
            "startLine": 1812,
            "startColumn": 9,
            "endColumn": 38
          }
        },
        "message": {
          "text": "Cmd"
        }
      }
    ]
  },
  {
    "ruleId": "py/overwritten-inherited-attribute",
    "rule": {
      "id": "py/overwritten-inherited-attribute",
      "index": 148,
      "toolComponent": {
        "index": 25
      }
    },
    "message": {
      "text": "Assignment overwrites attribute validate_one_type, which was previously defined in superclass [Cmd](1)."
    },
    "locations": [
      {
        "physicalLocation": {
          "artifactLocation": {
            "uri": "library/network_connections.py",
            "uriBaseId": "%SRCROOT%",
            "index": 1
          },
          "region": {
            "startLine": 2503,
            "startColumn": 9,
            "endLine": 2505,
            "endColumn": 10
          }
        }
      }
    ],
    "partialFingerprints": {
      "primaryLocationLineHash": "db8d057213974509:1",
      "primaryLocationStartColumnFingerprint": "0"
    },
    "relatedLocations": [
      {
        "id": 1,
        "physicalLocation": {
          "artifactLocation": {
            "uri": "library/network_connections.py",
            "uriBaseId": "%SRCROOT%",
            "index": 1
          },
          "region": {
            "startLine": 1812,
            "startColumn": 9,
            "endColumn": 38
          }
        },
        "message": {
          "text": "Cmd"
        }
      }
    ]
  },
  {
    "ruleId": "py/uninitialized-local-variable",
    "rule": {
      "id": "py/uninitialized-local-variable",
      "index": 162,
      "toolComponent": {
        "index": 25
      }
    },
    "message": {
      "text": "Local variable 'ac' may be used before it is initialized."
    },
    "locations": [
      {
        "physicalLocation": {
          "artifactLocation": {
            "uri": "library/network_connections.py",
            "uriBaseId": "%SRCROOT%",
            "index": 1
          },
          "region": {
            "startLine": 2451,
            "startColumn": 54,
            "endColumn": 56
          }
        }
      }
    ],
    "partialFingerprints": {
      "primaryLocationLineHash": "dd9fcbb08231241c:1",
      "primaryLocationStartColumnFingerprint": "37"
    }
  }
]

nhosoi · 2023-01-30T19:45:03Z

Hmmm, maybe I'd better close this pr since the network role does not have the codeql action added. What do you think, @richm? If so, sorry for the noise...

richm · 2023-01-30T19:51:24Z

Hmmm, maybe I'd better close this pr since the network role does not have the codeql action added. What do you think, @richm? If so, sorry for the noise...

I think the plan is for network to drop lgtm and use codeql, so they will need these fixes (unless they want to fix the code in some other way) - I guess it's up to @liangwen12year and the rest of the network team to decide what they want to do.

tyll · 2023-02-09T10:51:53Z

tests/unit/test_network_connections.py

@@ -3461,15 +3463,15 @@ def test_interface_name_ethernet_default(self):
        """Use the profile name as interface_name for ethernet profiles"""
        cons_without_interface_name = [{"name": "eth0", "type": "ethernet"}]
        connections = ARGS_CONNECTIONS.validate(cons_without_interface_name)
-        self.assertTrue(connections[0]["interface_name"] == "eth0")
+        self.assertEqual(connections[0]["interface_name"], "eth0")


https://docs.python.org/2.7/library/unittest.html#unittest.TestCase.assertIs this is new in Python 2.7 - in the past we needed to support Python 2.6 - is this not necessary anymore?

https://github.com/linux-system-roles/network/blob/main/contributing.md#where-to-start needs to be updated if we don't need to support Python 2.6 anymore.

I'm not sure if we still need to support 2.6, but if we don't, it should be a conscious, deliberate decision, in a separate PR. So for this change, please use a method that is compatible with 2.6

tyll · 2023-02-09T15:50:48Z

library/network_connections.py

@@ -818,12 +818,14 @@ def connection_compare(
            try:
                con_a.normalize()
            except Exception:
+                # It's already checked normalize is successful.


@thom311 what kind of exceptions are ignored here?

Do you mean, which exceptions normalize() is expected to throw? Probably they are all subtypes of GLib.Error. But I don't know, and I don't want to find out -- because even if I check now, it's not clear that this check will hold for the future. The code really wants to catch and ignore any errors (that is, all Exception types -- but not all BaseException).

The code seems pretty clear in that. This is not a bug but done unintentionally. The code wants to normalize the profile, and if that fails, ignore the error. Any error.

The new code comment seems not clear to me. The proper comment would be # We ignore any errors that might happen, which seems a pretty meaningless thing to say. This linter rule seems questionable to me, but OK, it needs to be worked around.

In the commit message, I would not say "fix issues" (but false positives or warnings) -- or does the patch also fix actual bugs? In that case, the actual bug fixes should be separate commits.

I mean what can go wrong with normalization that it can be ignored but the connection is still fine without normalization so that we can safely ignore the error.

In the commit message, I would not say "fix issues" (but false positives or warnings) -- or does the patch also fix actual bugs? In that case, the actual bug fixes should be separate commits.

Well, maybe "issues" means something quite benign (like a warning). In my ear, it sounds severe to fix "issues". I would not use the wording "fix issue" for adding a code comment. If the patch fixes actual issues, this patch should not be cluttered with low-severity changes. If it doesn't fix any bugs, the word "issue" seems unnecessarily alarming.

nhosoi · 2023-02-09T16:41:22Z

Thank you, @tyll, @richm.
I'm reverting the changes complained about by the py26, and py27 tests.

tyll · 2023-02-09T18:15:48Z

library/network_connections.py

@@ -2550,6 +2552,8 @@ def forget_nm_connection(self, path):
                ]
            )
        except Exception:
+            # Assume that NetworkManager is not present.


This is duplicating the doc string for the function, so IMHO it would be better to remove this and/or to properly ignore only the error cases when busctl is not available or if the NetworkManager dbus is not available instead of ignoring all exceptions. I feel guilty since this is technical debt from me. 🙈

@liangwen12year, @thom311, could you please take a look at this exception? Thanks!

(If it's easier for you to work on, may I just revert this change and open an issue?)

tests/unit/test_network_connections.py

tyll · 2023-02-09T18:20:10Z

tests/unit/test_network_connections.py

-        self.assertRaises(ValidationError, v.validate, value)
+        self.assertRaises(
+            network_lsr.argument_validator.ValidationError, v.validate, value
+        )


If others agree with this change, please put it into its own commit/PR, this seems to be a valid change that could be merged - also the other, similar lines. 👍

Is this change due to a CodeQL error? If so, did CodeQL suggest this as the correction?

CodeQL complains about duplicate import statements: Module 'network_lsr.argument_validator' is imported with both 'import' and 'import from'. And no, CodeQL did not give us suggestions.

https://github.com/linux-system-roles/network/security/code-scanning/13 has the suggestion to do

import network_lsr ValidationError = network_lsr.argument_validator.ValidationError

to avoid importing it twice.

tyll · 2023-02-09T18:25:20Z

library/network_connections.py

@@ -1493,8 +1495,7 @@ def check_activated(ac, dev):
            except AttributeError:
                ac_reason = None

-            if dev:
-                dev_state = dev.get_state()
+            dev_state = dev.get_state() if dev else None


I believe my previous comment was lost, if not, please excuse the duplicate.
CodeQL properly identifies this code as problematic. This change only makes CodeQL happy but does not really work towards refactoring the code to make it easier to understand. In the following lines dev and dev_state are used without checking whether they are None and I guess it is also not needed since there is some implicit understanding whether this function will actually be called with dev not being a true value. I would rather see this function being properly refactored to make full use of CodeQL and creating additional benefit for the future.

Thanks for the detailed explanation, @tyll ! Reverted.

tyll · 2023-02-09T18:28:44Z

module_utils/network_lsr/argument_validator.py

@@ -271,6 +271,7 @@ def _validate_impl(self, value, name):
                except Exception:
                    table = value
        except Exception:


@thom311 @liangwen12year what exceptions are we excepting here? Is it a ValueError from line 267? It seems like it could be excepted there specifically.

I don't think any exception can be raised here. The outer try/except should be dropped.

Ah, I see now. So table = value would be better in line 267, wouldn't it?

If the value is already the int type, then the function int() seems to me is not needed.

isinstance() does not check whether it's the same type, but of the type or a subtype.

the point of the code is of course to ensure that the result is really an int, and not a subtype.

"not needed", is if we never call the function with a subtype of int (bool is also a subtype of int, but already handled above).

I would leave it.

What's the problem if it is a subclass? Regarding bool, the check is there to avoid accepting boolean values but other subclasses are not rejected, so it is not clear to me why this would be useful.

the idea is that the validation+normalization returns ... normalized values. That is, int and not a subclass of an int that was passed as input to the validation.

Anyway, since nobody is actually subclassing int (aside bool, which is handled already), it doesn't have any effect.

Normalization can also mean to normalize it to int or a subclass of it.

library/network_connections.py - 'except' clause does nothing but pass and there is no explanatory comment. module_utils/network_lsr/argument_validator.py - 'except' clause does nothing but pass and there is no explanatory comment. module_utils/network_lsr/utils.py - Mixing implicit and explicit returns may indicate an error as implicit returns always return None. tests/unit/test_network_connections.py - Module 'network_lsr.argument_validator' is imported with both 'import' and 'import from'. - assertTrue(a == b) cannot provide an informative message. Using assertEqual(a, b) instead will give more informative messages. Signed-off-by: Noriko Hosoi <nhosoi@redhat.com>

module_utils/network_lsr/argument_validator.py

tyll · 2023-02-09T20:52:45Z

library/network_connections.py

@@ -818,12 +818,14 @@ def connection_compare(
            try:
                con_a.normalize()
            except Exception:
+                # We ignore any errors that might happen


This comment does not provide any additional value since it just describes the code. The comment should include the reason why it is the right thing to do this here.

This comment does not provide any additional value since it just describes the code. The comment should include the reason why it is the right thing to do this here.

#558 (comment)

The proper comment would be # We ignore any errors that might happen, which seems a pretty meaningless thing to say. This linter rule seems questionable to me, but OK, it needs to be worked around.

We could just say # Workaround CodeQL warning, then?

nhosoi · 2023-02-10T01:27:25Z

Thank you for the discussions, @tyll, @richm, @liangwen12year, @thom311.
Instead of discussing each warning in this PR, I believe it should be much faster and cleaner for an expert to clean up the code. So, I'm going to close this PR for now, and open an issue with the raw output from CodeQL pointing to this PR.

liangwen12year self-requested a review February 2, 2023 22:10