Opened 3 years ago

Closed 3 years ago

#5004 closed defect (fixed)

commandReader improper string constructor

Reported by: fdupont Owned by: fdupont
Priority: medium Milestone: Kea1.1-final
Component: Unclassified Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

CID 1372421: the string constructor is called with a char* which is not null-terminated.
I put it in 1.1-final because it is trivial to fix. Note the Coverity report has 3 new items.

Subtickets

Change History (7)

comment:1 Changed 3 years ago by fdupont

  • Owner set to fdupont
  • Status changed from new to accepted

comment:2 Changed 3 years ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing

Done. I did the 2 other new items too.

comment:3 Changed 3 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:4 follow-up: Changed 3 years ago by marcin

  • Owner changed from marcin to fdupont

Reviewed up to commit 679c5a777dd0acb1f7f08892a4c9473264d55335

src/lib/config/command_mgr.cc
Invalid indentation in lines 154 and 155. Also, you used tabs instead of spaces for indentation.

The changes made to the code look good.

src/lib/eval/tests/token_unittest.cc
Did you mean to use "unsigned int", rather than "unsigned" within the following loop:

for (unsigned i = 0; i < tuples_size; ++i) {
...
}

at line 300. Personally, I don't see an issue with using "unsigned"-only, but we normally use long form.

There is odd indentation in line 298.

nb_content in line 298 could be const.

src/lib/util/tests/socketsession_unittest.cc
Don't you think we need to "memset" recvbuf to 0 to get rid of the coverity warning? That would guarantee that the recvbuf output is null terminated after all?

On that matter, I think you haven't addressed another issue with:

EXPECT_EQ(string(TEST_DATA), string(recvbuf));

which creates a string from recvbuf, which is potentially not null terminated. It would be, if you used memset, but I'd rather see the same solution as you used in command_mgr.cc

comment:5 in reply to: ↑ 4 Changed 3 years ago by fdupont

  • Owner changed from fdupont to marcin

Replying to marcin:

Reviewed up to commit 679c5a777dd0acb1f7f08892a4c9473264d55335

src/lib/config/command_mgr.cc
Invalid indentation in lines 154 and 155. Also, you used tabs instead of spaces for indentation.

=> done.

The changes made to the code look good.

src/lib/eval/tests/token_unittest.cc
Did you mean to use "unsigned int", rather than "unsigned" within the following loop:

for (unsigned i = 0; i < tuples_size; ++i) {
...
}

at line 300. Personally, I don't see an issue with using "unsigned"-only, but we normally use long form.

=> unsigned alone means unsigned int but I changed for a size_t which is the type of tuples_size.

There is odd indentation in line 298.

nb_content in line 298 could be const.

=> fixed.

src/lib/util/tests/socketsession_unittest.cc
Don't you think we need to "memset" recvbuf to 0 to get rid of the coverity warning? That would guarantee that the recvbuf output is null terminated after all?

=> this is C++ so the array can be initialized with a = { }; .

On that matter, I think you haven't addressed another issue with:

EXPECT_EQ(string(TEST_DATA), string(recvbuf));

which creates a string from recvbuf, which is potentially not null terminated.

It would be, if you used memset, but I'd rather see the same solution as you used in command_mgr.cc

=> Add a size doesn't give the same result because it can embed a \0 in the C++ string.

BTW I don't believe a ChangeLog entry is required.

comment:6 Changed 3 years ago by marcin

  • Owner changed from marcin to fdupont

All changes look good. Please merge. I agree that there is no need for a ChangeLog entry.

comment:7 Changed 3 years ago by fdupont

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged (with eval parser files regen). Closing.

Note: See TracTickets for help on using tickets.