Skip to content

CID 1516688: Fix uninitialized member of AcceptOptions#10152

Merged
masaori335 merged 3 commits intoapache:masterfrom
masaori335:fix-10013
Sep 7, 2023
Merged

CID 1516688: Fix uninitialized member of AcceptOptions#10152
masaori335 merged 3 commits intoapache:masterfrom
masaori335:fix-10013

Conversation

@masaori335
Copy link
Contributor

@masaori335 masaori335 commented Aug 7, 2023 •
edited
Loading

Fix #10013

Update:

Cleanup AcceptOptions

  1. Remove the reset() function, because it's only called by the constructor.
  2. Remove the AcceptOptions.cc file because it has the reset() function only.
  3. Set in-class initializers to AcceptOptions members.
    IpAddr local_ip is an exception. Calling invalidate() is the same as the initialization of IpAddr.

trafficserver/include/tscore/ink_inet.h

Lines 1302 to 1308 in f9aa1e0

/// Make invalid.
self &
invalidate()
{
_family = AF_UNSPEC;
return *this;
}

trafficserver/include/tscore/ink_inet.h

Line 1317 in f9aa1e0

uint16_t _family = AF_UNSPEC; ///< Protocol family.

@masaori335 masaori335 added this to the 10.0.0 milestone Aug 7, 2023
@masaori335 masaori335 self-assigned this Aug 7, 2023
jpeach
jpeach reviewed Aug 7, 2023
View reviewed changes
@masaori335
Copy link
Contributor Author

masaori335 commented Aug 7, 2023

[approve ci freebsd]

jpeach
jpeach reviewed Aug 7, 2023
View reviewed changes
iocore/net/AcceptOptions.h Outdated
int accept_threads = -1;
/// Event type to generate on accept.
EventType etype;
EventType etype = ET_NET;
Copy link
Contributor

@jpeach jpeach Aug 7, 2023

Choose a reason for hiding this comment

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

I think you could remove this (and not have to include "I_Net.h") because it is never set to anything other than ET_NET.

Copy link
Contributor

@bryancall bryancall Aug 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I don't see it being set to anything else. You could get rid of the member variable and just use ET_NET in the code.

NetAccept::opt.etype is set to 0 in both NetAccept::do_blocking_accept() and NetAccept::acceptFastEvent() when setting ts.accept_thread to either 1 or 0 respectively.

jpeach
jpeach previously approved these changes Aug 7, 2023
View reviewed changes
maskit
maskit reviewed Aug 7, 2023
View reviewed changes
iocore/net/AcceptOptions.h Outdated
#include "tscore/ink_inet.h"

#include "I_Event.h"
#include "I_Net.h"
Copy link
Member

@maskit maskit Aug 7, 2023

Choose a reason for hiding this comment

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

Doesn't this bring a lot of dependency? I think that's the reason we have AcceptOptions.cc separately.

@moonchen What do you think?

@bryancall bryancall self-requested a review August 7, 2023 22:15
maskit
maskit previously requested changes Aug 7, 2023
View reviewed changes
Copy link
Member

@maskit maskit left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We need to think about I_Net.h. @bryancall and some others are going to look into it.

@moonchen
Copy link
Contributor

moonchen commented Aug 16, 2023

I'm for hard-coding the etype to ET_NET. If we want to keep it as a variable, I suggest we pass in etype from the constructor of AcceptOptions so that ET_NET is not a dependency of AcceptOptions.

@masaori335 masaori335 force-pushed the fix-10013 branch 2 times, most recently from 35f7896 to 059df6a Compare August 28, 2023 01:08
@masaori335
Copy link
Contributor Author

masaori335 commented Aug 28, 2023

Removed AcceptOptions::etype and rebased on the latest master.

@bryancall bryancall requested a review from maskit September 5, 2023 16:02
@maskit maskit dismissed their stale review September 5, 2023 21:27

Looks like the issue was resolved.

@masaori335 masaori335 merged commit 9d3d3c2 into apache:master Sep 7, 2023
cmcfarlen pushed a commit to cmcfarlen/trafficserver that referenced this pull request Jun 3, 2024
@zwoop
Merge remote-tracking branch 'asf/master'
cdd35f9 
* asf/master: (22 commits)
  fix: check whether a protocol is enabled during the length calculation in create_npn_advertisement (apache#10381)
  Coverity 1518612: Remove dead code (apache#10384)
  prefetch_cmcd: make autests more robust by removing need for gold file wildcard (apache#10382)
  Give a chance to send a response before receiving next request on H2 (apache#9997)
  CID 1516688: Fix uninitialized member of AcceptOptions (apache#10152)
  Fix slice head request memory issue (apache#10285)
  Fixes the TSMgmt metrics APIs for new API metrics (apache#10379)
  Minor parent.config a/an change (apache#10372)
  Allow DbgCtl tag to be set after instance construction. (apache#10375)
  Fix more build dep issues, for later PRs to work (apache#10376)
  money_trace cid 1518569: string not null terminated (apache#10373)
  Fix a couple of Coverity issues in health check plugin, around filenames (apache#10371)
  Fixes some build issues that happens with  other changes (apache#10374)
  Eliminate unreachable code covered by switch default (apache#10370)
  Add tests for disk failure (apache#10192)
  Disable copying/moving for DbgCtl. (apache#10321)
  Cmake autest (apache#10327)
  cmake: add unit tests from mgmt/rpc (apache#10366)
  Adjust CMakeLists with git worktree (apache#10298)
  Fix example plugins build (apache#10326)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bryancall bryancall bryancall approved these changes

@jpeach jpeach jpeach left review comments

@maskit maskit Awaiting requested review from maskit

Assignees

@masaori335 masaori335

Labels

Cleanup Coverity Network

Projects

None yet

Milestone

10.0.0

Development

Successfully merging this pull request may close these issues.

CID 1516688: Uninitialized members (UNINIT_CTOR)

5 participants

@masaori335 @moonchen @jpeach @maskit @bryancall