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.

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

test, osd: fix some coverity issues #13293

Merged
merged 2 commits into from Mar 23, 2017

Conversation

Projects
None yet
4 participants
@Liuchang0812
Copy link
Contributor

Liuchang0812 commented Feb 7, 2017

@liewegas @tchaikov take a look, please

@Liuchang0812 Liuchang0812 changed the title [DNM]cleanup: fix some coverity issues cleanup: fix some coverity issues Feb 8, 2017

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Feb 10, 2017

jenkins test this please

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Feb 20, 2017

is it OK to merge?

@@ -1728,7 +1728,7 @@ void PG::activate(ObjectStore::Transaction& t,
// share past_intervals if we are creating the pg on the replica
// based on whether our info for that peer was dne() *before*
// updating pi.history in the backfill block above.
if (needs_past_intervals)
if (m && needs_past_intervals)

This comment has been minimized.

Copy link
@tchaikov

tchaikov Feb 20, 2017

Contributor

could you prefix the title of your commit message with the subcomponent your are changing ? see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#3-describe-your-changes, in this case, it would be "osd/PG: ".

@@ -144,17 +144,24 @@ int main(int args, char **argv)
std::cerr << "Error " << ret << " in cluster.init" << std::endl;
return ret;
}

This comment has been minimized.

Copy link
@tchaikov

tchaikov Feb 20, 2017

Contributor

could you prefix the title of your commit message with the subcomponent your are changing ? see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#3-describe-your-changes, in this case, it would be "test/multi_stress_watch: ".

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Feb 20, 2017

Author Contributor

OK

ostringstream sa;
sa << i->addr;
strncpy(ow.addr, sa.str().c_str(), 256);
ostringstream ss;

This comment has been minimized.

Copy link
@tchaikov

tchaikov Feb 20, 2017

Contributor

why would you want to s/sa/ss/ ?

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Feb 20, 2017

Author Contributor

well, I use ss for stringstram class. I will reset this change.

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Feb 20, 2017

Author Contributor

done

ostringstream ss;
ss << i->addr;
strncpy(ow.addr, ss.str().c_str(), 256);
ow.addr[255] = '\0'; // make coverity happy

This comment has been minimized.

Copy link
@tchaikov

tchaikov Feb 20, 2017

Contributor

strncpy() does include the terminating \0. and ss.str().c_str() evaluates a null-terminated c string.

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Feb 20, 2017

Author Contributor

Yeah, I know that, but coverity doesn't know. just make coverity happy here

This comment has been minimized.

Copy link
@tchaikov

tchaikov Feb 20, 2017

Contributor

if fixing the coverity issue helps us to improve the code, i am happy to do so. but this change does not make sense in the sense of correctness or readability or feature.

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Feb 20, 2017

Author Contributor

ok, i will reset this file.

ldout(cct, 10) << __func__ << " from " << p->first << " to "
<< r->first << dendl;
if (r != new_pools.end()) {
ldout(cct, 10) << __func__ << " from " << p->first << " to "

This comment has been minimized.

Copy link
@tchaikov

tchaikov Feb 20, 2017

Contributor

no need to check r != new_pools.end(), should just put

	ldout(cct, 10) << __func__ << " from " << p->first << " to "
		       << *q << dendl;

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Feb 20, 2017

Author Contributor

done

@@ -524,8 +524,10 @@ void RGW_SWIFT_Auth_Get::execute()
protocol = "https";
} else {
server_port = s->info.env->get("SERVER_PORT");
add_port = (strcmp(server_port, "80") != 0);
protocol = "http";
if (server_port) {

This comment has been minimized.

Copy link
@tchaikov

tchaikov Feb 20, 2017

Contributor

the url should always start with a protocol, either "https" or "http" here.

@yehudasa shall we have a default 80 "SERVER_PORT" here, so we can default to "http" here?

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Feb 20, 2017

Author Contributor

CGI Spec said that CGI will always have one of "https" or "http". Just make coverity happy.

This comment has been minimized.

Copy link
@tchaikov

tchaikov Feb 20, 2017

Contributor

@Liuchang0812 that's exactly my concern. we can not leave a null protocol here.

This comment has been minimized.

Copy link
@tchaikov

tchaikov Feb 20, 2017

Contributor

this is not addressed, we should have a default value for protocol. what if neither "SERVER_PORT_SECURE" nor "SERVER_PORT" is specified?

swift_url = protocol; at 06b7d5c#diff-e99d9bc966465b3d31f9909075b6ecc8L536 will crash.

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Feb 20, 2017

Author Contributor

hi, @tchaikov, I prefer to add this check, and add a assert(0 == "neither HTTP nor HTTPS is provided"). It makes code safer and increase readability for who doesn't familiar with CGI Spec.

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Feb 20, 2017

Author Contributor
the url should always start with a protocol, either "https" or "http" here.

@yehudasa shall we have a default 80 "SERVER_PORT" here, so we can default to "http" here?

That giving protocol a default value is also a good solution.

This comment has been minimized.

Copy link
@tchaikov

tchaikov Feb 20, 2017

Contributor

what if neither "SERVER_PORT_SECURE" nor "SERVER_PORT" is specified

i am just not sure if this happens in real world or not.

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Feb 20, 2017

Author Contributor

I'm not sure.
CGI RFC[1] said that:

 Note that this variable MUST be set, even if the port is the default
   port for the scheme and could otherwise be omitted from a URI.

[1]. https://tools.ietf.org/html/rfc3875#section-4.1.15

This comment has been minimized.

Copy link
@tchaikov

tchaikov Feb 20, 2017

Contributor

let's add an assert then

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Feb 21, 2017

Author Contributor

OK, It's necessary

@@ -524,8 +524,10 @@ void RGW_SWIFT_Auth_Get::execute()
protocol = "https";
} else {
server_port = s->info.env->get("SERVER_PORT");
add_port = (strcmp(server_port, "80") != 0);
protocol = "http";
if (server_port) {

This comment has been minimized.

Copy link
@tchaikov

tchaikov Feb 20, 2017

Contributor

could you prefix the title of your commit message with the subcomponent your are changing ? see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#3-describe-your-changes, in this case, it would be "rgw: "

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Feb 20, 2017

Author Contributor

OK

@@ -524,8 +524,10 @@ void RGW_SWIFT_Auth_Get::execute()
protocol = "https";
} else {
server_port = s->info.env->get("SERVER_PORT");
add_port = (strcmp(server_port, "80") != 0);
protocol = "http";
if (server_port) {

This comment has been minimized.

Copy link
@tchaikov

tchaikov Feb 20, 2017

Contributor

could you prefix the title of your commit message with the subcomponent your are changing ? see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#3-describe-your-changes, in this case, it would be "rgw: "

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Feb 20, 2017

@tchaikov thanks for reviewing. I will update it soon

@Liuchang0812 Liuchang0812 force-pushed the Liuchang0812:cleanup-coverity branch from 484d35e to 06b7d5c Feb 20, 2017

@Liuchang0812 Liuchang0812 changed the title cleanup: fix some coverity issues rgw, test, osd: fix some coverity issues Feb 20, 2017

@@ -524,8 +524,10 @@ void RGW_SWIFT_Auth_Get::execute()
protocol = "https";
} else {
server_port = s->info.env->get("SERVER_PORT");
add_port = (strcmp(server_port, "80") != 0);
protocol = "http";
if (server_port) {

This comment has been minimized.

Copy link
@tchaikov

tchaikov Feb 20, 2017

Contributor

this is not addressed, we should have a default value for protocol. what if neither "SERVER_PORT_SECURE" nor "SERVER_PORT" is specified?

swift_url = protocol; at 06b7d5c#diff-e99d9bc966465b3d31f9909075b6ecc8L536 will crash.

@Liuchang0812 Liuchang0812 force-pushed the Liuchang0812:cleanup-coverity branch 2 times, most recently from 7f9ed99 to 2551382 Feb 21, 2017

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Feb 21, 2017

@tchaikov I have added a assert line. mind taking a look?

@@ -524,8 +525,13 @@ void RGW_SWIFT_Auth_Get::execute()
protocol = "https";
} else {
server_port = s->info.env->get("SERVER_PORT");
add_port = (strcmp(server_port, "80") != 0);
protocol = "http";
if (server_port) {

This comment has been minimized.

Copy link
@tchaikov

tchaikov Feb 21, 2017

Contributor

The SERVER_PORT variable MUST be set to the TCP/IP port number on
which this request is received from the client. This value is used
in the port part of the Script-URI.

  SERVER_PORT = server-port
  server-port = 1*digit

Note that this variable MUST be set, even if the port is the default
port for the scheme and could otherwise be omitted from a URI.

iiuc, we can just assert(server_port) before add_port = (strcmp(server_port, "80") != 0);.

@tchaikov

This comment has been minimized.

Copy link
Contributor

tchaikov commented Feb 21, 2017

and please split your PR into smaller commits which addressing different issues in different subcomponents.

@Liuchang0812 Liuchang0812 force-pushed the Liuchang0812:cleanup-coverity branch from 2551382 to 6fa29d4 Feb 21, 2017

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Feb 21, 2017

@tchaikov I have splited PR into 3 commits: osd, rgw, test, and added ceph_assert(server_port). thank you very much.

@tchaikov tchaikov requested a review from yehudasa Feb 21, 2017

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Mar 1, 2017

@yehudasa mind taking a look? thank you very much

@Liuchang0812 Liuchang0812 force-pushed the Liuchang0812:cleanup-coverity branch from 6fa29d4 to 6e17b8a Mar 20, 2017

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Mar 20, 2017

@tchaikov I droped commit about rgw_swift_auth. I will add it in subsequent PR. We could merge other commits.

osd: fix null dereferenced in PG, fix dereferencing invalid iterator
Fixes: CID#1394843
Fixes: CID#1160830

Signed-off-by: liuchang0812 <liuchang0812@gmail.com>

@Liuchang0812 Liuchang0812 changed the title rgw, test, osd: fix some coverity issues test, osd: fix some coverity issues Mar 20, 2017

ret = cluster.conf_read_file(NULL);
if (ret) {
std::cerr << "Error " << ret << " in cluster.conf_read_file" << std::endl;
return ret;
}

This comment has been minimized.

Copy link
@tchaikov

tchaikov Mar 20, 2017

Contributor

please remove the formatting only changes.

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Mar 20, 2017

Author Contributor

ok

test: check return value in test/multi_stress_watch
Fixes: CID#716871

Signed-off-by: liuchang0812 <liuchang0812@gmail.com>

@Liuchang0812 Liuchang0812 force-pushed the Liuchang0812:cleanup-coverity branch from 6e17b8a to dae052f Mar 20, 2017

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Mar 20, 2017

ChangLog:

  1. update code based on @tchaikov
  2. rebase to master, and repushed.

@tchaikov tchaikov added the needs-qa label Mar 20, 2017

@yuriw

This comment has been minimized.

@yuriw yuriw merged commit c54eb54 into ceph:master Mar 23, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.