Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

(#1694605) Backport/fixes for issues found by LGTM on RHEL 7 #311

Closed
wants to merge 21 commits into from
Closed

(#1694605) Backport/fixes for issues found by LGTM on RHEL 7 #311

wants to merge 21 commits into from

Conversation

mrc0mmand
Copy link
Collaborator

@mrc0mmand mrc0mmand commented Feb 27, 2019 •
edited

This PR attempts to fix errors & warnings found by LGTM. Recommendations are currently ignored, as it would make this PR unnecessarily big.

Note: some of these patches are not even in upstream (mostly because the particular code was completely rewritten in upstream).

Unresolved errors/warnings:

  • [error] src/shared/pager.c [link] - pager-related stuff has been significantly refactored in upstream, needs further investigation
  • [ignored] src/libsystemd/sd-bus/bus-bloom.h [link] - dropped from upstream along with kdbus
  • src/shared/strv.c [link] - strv_join_quoted was dropped from upstream (replaced by a different approach) without resolving this issue; would require some, possibly substantial, refactoring
  • [ignored] src/bootchart/* [link] - dropped from upstream (systemd/systemd@232c84b)
  • src/core/execute.c [link] - process_execute has been refactored to support longer process names (systemd/systemd@9bfaffd), not sure if it's worth backporting
  • [ignored] src/gudev/* - dropped from upstream (systemd/systemd@2375607)
  • src/shared/fw-util.c [link] - renamed to src/shared/firewall-util.c, however, not yet fixed in upstream

The [ignored] rules should hopefully take effect once this PR is merged.

@lnykryn
Copy link
Owner

lnykryn commented Feb 27, 2019

This pull request fixes 13 alerts when merging 4df22c7 into a781c22 - view on LGTM.com

fixed alerts:

  • 6 for Assignment where comparison was intended
  • 4 for Potentially overflowing call to snprintf
  • 3 for Use of potentially dangerous function

Comment posted by LGTM.com

@lnykryn
Copy link
Owner

lnykryn commented Feb 27, 2019

This pull request fixes 15 alerts when merging 73386a7 into a781c22 - view on LGTM.com

fixed alerts:

  • 6 for Assignment where comparison was intended
  • 4 for Potentially overflowing call to snprintf
  • 3 for Use of potentially dangerous function
  • 2 for Comparison result is always the same

Comment posted by LGTM.com

@mrc0mmand mrc0mmand force-pushed the rhel7-lgtm-fixes branch 2 times, most recently from 375afff to 368d4f4 Compare February 27, 2019 18:19
@lnykryn
Copy link
Owner

lnykryn commented Feb 27, 2019

This pull request fixes 23 alerts when merging 368d4f4 into a781c22 - view on LGTM.com

fixed alerts:

  • 7 for Comparison result is always the same
  • 6 for Assignment where comparison was intended
  • 4 for Potentially overflowing call to snprintf
  • 3 for Use of potentially dangerous function
  • 3 for Implicit downcast from bitfield

Comment posted by LGTM.com

@lnykryn
Copy link
Owner

lnykryn commented Feb 27, 2019

This pull request introduces 1 alert and fixes 25 when merging 300a5dd into a781c22 - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

fixed alerts:

  • 8 for Comparison result is always the same
  • 6 for Assignment where comparison was intended
  • 4 for Potentially overflowing call to snprintf
  • 3 for Use of potentially dangerous function
  • 3 for Implicit downcast from bitfield
  • 1 for Ambiguously signed bit-field member

Comment posted by LGTM.com

@lnykryn
Copy link
Owner

lnykryn commented Feb 28, 2019

This pull request fixes 26 alerts when merging 5a5a676 into a781c22 - view on LGTM.com

fixed alerts:

  • 9 for Comparison result is always the same
  • 6 for Assignment where comparison was intended
  • 4 for Potentially overflowing call to snprintf
  • 3 for Use of potentially dangerous function
  • 3 for Implicit downcast from bitfield
  • 1 for Ambiguously signed bit-field member

Comment posted by LGTM.com

@lnykryn
Copy link
Owner

lnykryn commented Feb 28, 2019

This pull request fixes 28 alerts when merging ba2518d into a781c22 - view on LGTM.com

fixed alerts:

  • 11 for Comparison result is always the same
  • 6 for Assignment where comparison was intended
  • 4 for Potentially overflowing call to snprintf
  • 3 for Use of potentially dangerous function
  • 3 for Implicit downcast from bitfield
  • 1 for Ambiguously signed bit-field member

Comment posted by LGTM.com

@mrc0mmand mrc0mmand force-pushed the rhel7-lgtm-fixes branch 3 times, most recently from a7b5463 to 70c9246 Compare February 28, 2019 14:01
@lnykryn
Copy link
Owner

lnykryn commented Feb 28, 2019

This pull request fixes 35 alerts when merging 70c9246 into a781c22 - view on LGTM.com

fixed alerts:

  • 11 for Comparison result is always the same
  • 6 for Missing variable declaration
  • 6 for Assignment where comparison was intended
  • 4 for Potentially overflowing call to snprintf
  • 3 for Use of potentially dangerous function
  • 3 for Implicit downcast from bitfield
  • 1 for Unneeded defensive code
  • 1 for Ambiguously signed bit-field member

Comment posted by LGTM.com

@mrc0mmand mrc0mmand changed the title [WIP] Backport/fixes for issues found by LGTM on RHEL 7 Backport/fixes for issues found by LGTM on RHEL 7 Feb 28, 2019
@mrc0mmand mrc0mmand added needs-exhaustive-review needs-bz Bugzilla needs to be filed and removed dont-merge labels Feb 28, 2019
@mrc0mmand
Copy link
Collaborator Author

Even though this PR doesn't fix all identified warnings, I'd review and merge it in its current state, as stuff's getting too hard to follow.

@jsynacek
Copy link
Collaborator

IMHO all the commits need to include "Resolves:" or "Related:", otherwise it won't be possible to merge them downstream.

@mrc0mmand
Copy link
Collaborator Author

@jsynacek thanks for pointing it out, it completely slipped my mind. Should be fixed now.

@mrc0mmand mrc0mmand changed the title Backport/fixes for issues found by LGTM on RHEL 7 (#1694605) Backport/fixes for issues found by LGTM on RHEL 7 Apr 1, 2019
@mrc0mmand mrc0mmand removed the needs-bz Bugzilla needs to be filed label Apr 1, 2019
poettering and others added 20 commits May 3, 2019 12:50
@poettering @mrc0mmand
timedate: use gmtime_r() and localtime_r()
20ddf9e 
gmtime() and localtime() operate on a static buffer. let's avoid this,
as we never know whether some library might use these calls in some
backrgound thread.

Discovered by lgtm:

https://lgtm.com/projects/g/systemd/systemd/
(cherry picked from commit e46acb7)

Resolves: #1694605
@evverx @mrc0mmand
sd-bus: stop using the result of an assignment as an operand of &&
d5ddc4d 
This makes OBJECT_PATH_FOREACH_PREFIX consistent with PATH_FOREACH_PREFIX
and also fixes 7 alerts reported by LGTM at
https://lgtm.com/projects/g/systemd/systemd/snapshot/ac0a08700344a5690803df8dd80e8bb5013184a5/files/src/libsystemd/sd-bus/bus-objects.c?sort=name&dir=ASC&mode=heatmap&showExcluded=true#V1383

(cherry picked from commit 283c265)

Resolves: #1694605
@keszybz @mrc0mmand
journal-upload: add asserts that snprintf does not return an error
46d51af 
LGMT complains:
> The size argument of this snprintf call is derived from its return value,
> which may exceed the size of the buffer and overflow.

Let's make sure that r is non-negative. (This shouldn't occur unless the format
string is borked, so let's just add an assert.)
Then, let's reorder the comparison to avoid the potential overflow.

(cherry picked from commit 91db8ed)

Resolves: #1694605
@yuwata @mrc0mmand
analyze: actually select longest activated-time of services
0d8c472 
Discovered by LGTM.

(cherry picked from commit e681315)

Resolves: #1694605
@mrc0mmand
pid1: remove unnecessary error reassignment
5db1699 
r is always >= 0 so let's drop the unnecessary condition
to make LGTM happy

Based on commit 7c3733d

Resolves: #1694605
@mrc0mmand
core: drop unnecessary condition
66c01ad 
n is always > 0 as all goto statements for this section are already
guarded by a (n > 0) condition. Let's drop it from the fail section
to make LGTM happy.

Relevant commits:
11a30ce
59eeb84

Resolves: #1694605
@mrc0mmand
core: drop unnecessary condition
57f981f 
In this case ret is always >= 0 as it still contains the initialization
value (i.e. 0). Let's drop the unnecessary condition to make LGTM happy.

Relevant commit: efdb023

Resolves: #1694605
@mrc0mmand
import: drop unnecessary condition
70cdf3d 
Relevant commit: d61b34f

Resolves: #1694605
@keszybz @mrc0mmand
journal-verify: add comment and silence LGTM warning
5bfc4cc 
(cherry picked from commit 0a58733)

Resolves: #1694605
@mrc0mmand
journal: drop redundant condition
d04ebf0 
Relevant commit: 29bfb68

Resolves: #1694605
@mrc0mmand
lldp: set correct state for processing
9d5d17b 
ldp_receive_frame after correct processing of the packet the state
should be LLDP_AGENT_RX_WAIT_FOR_FRAME not LLDP_AGENT_RX_UPDATE_INFO.

(cherry picked from commit 9bb1bff)

Resolves: #1694605
@yuwata @mrc0mmand
sd-bus: fix implicit downcast of bitfield reported by LGTM
dec5eae 
(cherry picked from commit affaed1)

Resolves: #1694605
@mrc0mmand
tree-wide: use 'signed int' instead of 'int' for bit field variables
ef5af27 
Suggested by LGTM: https://lgtm.com/rules/1506024027114/

(cherry picked from commit 845d247)

Resolves: #1694605
@mrc0mmand
machinectl: check correct FD for a return value
d55eb6a 
Found by LGTM.

Resolves: #1694605
@mrc0mmand
shared/install: avoid overwriting 'r' counter with a partial result
e5f0d25 
We want to store either the first error or the total number of changes in 'r'.
Instead, we were overwriting this with the return value from
install_info_traverse().

LGTM complained later in the loop that:
> Comparison is always true because r >= 0.

Relevant commit: 459500a

Resolves: #1694605
@mrc0mmand
systemctl: set_put() can't return (-)EEXIST
b0cceb7 
set_put() returns 0 if the key already exists, 1 if the entry
was inserted successfully, and -ENOMEM otherwise.

See: set_put(), hashmap_base_put_boldly()

Found by LGTM.

Resolves: #1694605
@keszybz @mrc0mmand
systemctl: remove dead check
1a4ac4a 
r could never be less than zero.

CID #1271350.

(cherry picked from commit 2558691)

Resolves: #1694605
@yuwata @mrc0mmand
journal-gateway: explicitly declare local variables
54fdba0 
Suggested by LGTM.

(cherry picked from commit c497e44)

Resolves: #1694605
@yuwata @mrc0mmand
journal-gateway: use localStorage["cursor"] only when it has valid value
9541afe 
Discovered by LGTM.

(cherry picked from commit 944072f)

Resolves: #1694605
@mrc0mmand
lgtm: ignore files dropped in upstream
f47b42d 
Resolves: #1694605
@mrc0mmand
lgtm: suppress several constant comparisons
d75397f 
Even though LGTM is right is these cases, fixing it would require
substantial refactoring in some cases, so it's better to leave them
here (at least for RHEL 7).

Resolves: #1694605
rhel-only
@lnykryn
Copy link
Owner

lnykryn commented May 3, 2019

This pull request fixes 38 alerts when merging d75397f into dd004bc - view on LGTM.com

fixed alerts:

  • 11 for Comparison result is always the same
  • 6 for Missing variable declaration
  • 6 for Assignment where comparison was intended
  • 6 for Use of potentially dangerous function
  • 4 for Potentially overflowing call to snprintf
  • 3 for Implicit downcast from bitfield
  • 1 for Unneeded defensive code
  • 1 for Ambiguously signed bit-field member

Comment posted by LGTM.com

@mrc0mmand mrc0mmand mentioned this pull request May 3, 2019
Merged
@mrc0mmand
Copy link
Collaborator Author

Closed in favor of redhat-plumbers/systemd-rhel7#2

@mrc0mmand mrc0mmand closed this May 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msekletar msekletar Awaiting requested review from msekletar

@jsynacek jsynacek Awaiting requested review from jsynacek

Assignees
No one assigned
Labels
needs-bz Bugzilla needs to be filed postponed rhel7
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@mrc0mmand @lnykryn @jsynacek @poettering @evverx @keszybz @yuwata