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

static analyzer fixes #352

Merged
merged 11 commits into from
Apr 11, 2024
Merged

static analyzer fixes #352

merged 11 commits into from
Apr 11, 2024

Conversation

aws-nslick
Copy link
Contributor

@aws-nslick aws-nslick commented Feb 22, 2024 •
edited
Loading

Description of changes:

a15e2c7 nit: silence clangsa warnings on pointer mismatch
ff22381 nit: remove extraneous assignment to self
7ae42c7 nit: test: unit: fix early-return leaks
2eacffd fix: net: strlen(NULL) is UB
9e79a7f fix: net: get_provider: always set return count
1cc9939 fix: topo: dont ignore errno
fe829de fix: api: initialize ofi_accept return value

Fixes various reports found in clang-static-analyzer/CodeChecker reporting. Others remain.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@aws-nslick aws-nslick marked this pull request as ready for review February 22, 2024 23:11
@aws-nslick aws-nslick requested review from rauteric, bwbarrett and a team as code owners February 22, 2024 23:11
bwbarrett
bwbarrett requested changes Feb 22, 2024
View reviewed changes
src/nccl_ofi_net.c Outdated Show resolved Hide resolved
src/nccl_ofi_net.c Outdated Show resolved Hide resolved
@AvivBenchorin AvivBenchorin added BuildTriggerRequest CI build will be triggered when this label is set and removed BuildTriggerRequest CI build will be triggered when this label is set labels Mar 3, 2024
@aws-nslick aws-nslick requested a review from rajachan as a code owner March 11, 2024 21:19
@aws-nslick aws-nslick force-pushed the clangsa-fixes branch 2 times, most recently from 0f501a1 to 9b5f632 Compare March 11, 2024 23:25
@AvivBenchorin AvivBenchorin added BuildTriggerRequest CI build will be triggered when this label is set and removed BuildTriggerRequest CI build will be triggered when this label is set labels Mar 12, 2024
rauteric
rauteric reviewed Mar 21, 2024
View reviewed changes
src/nccl_ofi_topo.c Show resolved Hide resolved
src/nccl_ofi_net.c Show resolved Hide resolved
src/nccl_ofi_net.c Show resolved Hide resolved
src/nccl_ofi_net.c Show resolved Hide resolved
tests/unit/scheduler.c Show resolved Hide resolved
@rajachan rajachan added BuildTriggerRequest CI build will be triggered when this label is set and removed BuildTriggerRequest CI build will be triggered when this label is set labels Mar 21, 2024
@rajachan
Copy link
Member

rajachan commented Apr 9, 2024

CC nccl_ofi_rdma.lo
nccl_ofi_net.c:389:3: error: use of undeclared identifier 'dev_props'
dev_props.mr_scope = NCCL_OFI_MR_SCOPE_ENDPOINT;
^
nccl_ofi_net.c:392:3: error: use of undeclared identifier 'dev_props'
dev_props.mr_scope = NCCL_OFI_MR_SCOPE_DOMAIN;
^
2 errors generated.

Build failures look real.

@aws-nslick
fix(api): initialize ofi_accept return value
50a56a3 
when *recvComm is NULL here, the compiler was under no obligation to
initialize this to any particular value, we must initialize it
explicitly.
@aws-nslick
fix(topo): dont ignore errno
2a24ca8 
if errno is indicated here, the iterator referenced further down
contains garbage. Exit early when errno < 0.
@aws-nslick
fix(net): strlen(NULL) is UB
f9039a3 
strlen(NULL) is undefined and may not necessarily return 0. Check
whether the pointer is set before calling strlen here.
@aws-nslick aws-nslick force-pushed the clangsa-fixes branch 2 times, most recently from aad0bb5 to b6d4e88 Compare April 9, 2024 22:57
bwbarrett
bwbarrett previously approved these changes Apr 10, 2024
View reviewed changes
src/nccl_ofi_ofiutils.c Outdated Show resolved Hide resolved
@rajachan rajachan added BuildTriggerRequest CI build will be triggered when this label is set and removed BuildTriggerRequest CI build will be triggered when this label is set labels Apr 10, 2024
@aws-nslick
fix: sendrecv: validate user mr_key_size
5425586 
The cast to size_t here was useless because it was performed on the
shift product, rather than the inputs, so this did not widen it.

Fix this casting issue and also check that we would not shift beyond a
size_t (error-out instead).
@aws-nslick
fix: make nccl_net_ofi_query_provider_capabilities const-correct
69053f0
@aws-nslick
fix: ofiutils: always write num_prov_infos
cf67fb9
@aws-nslick
fix(topo): use correct size_t format specifier
205bbdb
@aws-nslick
build: .gitignore: add clion folder
2652878
rajachan
rajachan approved these changes Apr 11, 2024
View reviewed changes
src/nccl_ofi_rdma.c Show resolved Hide resolved
@rajachan rajachan merged commit 883d7c7 into aws:master Apr 11, 2024
7 checks passed
@aws-nslick aws-nslick deleted the clangsa-fixes branch May 15, 2024 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rauteric rauteric rauteric left review comments

@rajachan rajachan rajachan approved these changes

@bwbarrett bwbarrett bwbarrett left review comments

Assignees
No one assigned
Labels
BuildTriggerRequest CI build will be triggered when this label is set
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aws-nslick @rajachan @rauteric @bwbarrett @AvivBenchorin