Fixes for issues found during SCA #21419

dalgaaf · 2018-04-13T14:45:01Z

Fixes for:

memory leaks (delete, realloc errors)
format qualifiers
boundary checks
fix parameter to variadic function calls
fix uninitialized variables

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

dillaman · 2018-04-13T15:03:56Z

src/librbd/api/Image.cc

@@ -274,6 +274,7 @@ int Image<I>::deep_copy(I *src, librados::IoCtx& dest_md_ctx,
  r = dest->state->open(false);
  if (r < 0) {
    lderr(cct) << "failed to read newly created header" << dendl;
+    delete dest;


👎 -- ImageState::open will automatically delete the ImageCtx upon failure [1]

[1] https://github.com/ceph/ceph/blob/master/src/librbd/ImageState.cc#L254

dillaman · 2018-04-13T15:04:04Z

src/librbd/internal.cc

@@ -1772,6 +1772,7 @@ bool compare_by_name(const child_info_t& c1, const child_info_t& c2)
    r = dest->state->open(false);
    if (r < 0) {
      lderr(cct) << "failed to read newly created header" << dendl;
+      delete dest;


👎 -- ImageState::open will automatically delete the ImageCtx upon failure [1]

[1] https://github.com/ceph/ceph/blob/master/src/librbd/ImageState.cc#L254

tchaikov · 2018-04-13T15:14:44Z

src/client/hypertable/CephBroker.cc

@@ -200,22 +200,22 @@ void CephBroker::read(ResponseCallbackRead *cb, uint32_t fd, uint32_t amount) {

  if (!m_open_file_map.get(fd, fdata)) {
    char errbuf[32];
-    sprintf(errbuf, "%d", fd);
+    sprintf(errbuf, "%u", fd);


we'd better to print an uint32_t using the macro of PRIu32 instead of %u, which works on platforms with 32bit integers but not on some other platforms.

tchaikov · 2018-04-13T15:17:25Z

src/common/TrackedOp.cc

@@ -152,7 +152,7 @@ OpTracker::OpTracker(CephContext *cct_, bool tracking, uint32_t num_shards):
  lock("OpTracker::lock"), cct(cct_) {
    for (uint32_t i = 0; i < num_optracker_shards; i++) {
      char lock_name[32] = {0};
-      snprintf(lock_name, sizeof(lock_name), "%s:%d", "OpTracker::ShardedLock", i);
+      snprintf(lock_name, sizeof(lock_name), "%s:%u", "OpTracker::ShardedLock", i);


use PRIu32 if you please.

tchaikov · 2018-04-13T15:17:33Z

src/osd/OSD.h

@@ -1733,7 +1733,7 @@ class OSD : public Dispatcher,
 	auto &&sdata = osd->shards[i];

 	char queue_name[32] = {0};
-	snprintf(queue_name, sizeof(queue_name), "%s%d", "OSD:ShardedOpWQ:", i);
+	snprintf(queue_name, sizeof(queue_name), "%s%u", "OSD:ShardedOpWQ:", i);


tchaikov · 2018-04-13T15:25:37Z

src/msg/async/EventKqueue.cc

-                    sizeof(struct SaveEvent)*newsize);
-    if (!sav_events) {
+    void *_realloc = NULL;
+    if ((_realloc = realloc(sav_events, izeof(struct SaveEvent)*newsize)) == NULL) {


s/izeof/sizeof/, could use "if with initialization" introduced by C++17, like

if (void* new_events = realloc(sav_events, sizeof(struct SaveEvent)*newsize)); !new_events) { lderr(cct) << __func__ << " unable to realloc memory: " << cpp_strerror(errno) << dendl; free(sav_events); return -ENOMEM; } else { sav_events = new_events; }

Strange, it compiled here fine.

tchaikov · 2018-04-13T15:26:20Z

src/rgw/rgw_common.h

@@ -1048,7 +1048,7 @@ struct rgw_bucket {
        uint64_t id;
        decode(id, bl);
        char buf[16];
-        snprintf(buf, sizeof(buf), "%llu", (long long)id);
+        snprintf(buf, sizeof(buf), "%" PRIu64 , id);


remove the space before ","

tchaikov · 2018-04-13T15:27:43Z

src/client/hypertable/CephBroker.cc

    cb->error(Error::DFSBROKER_BAD_FILE_HANDLE, errbuf);
    return;
  }

  int r;
  if ((r = ceph_fsync(cmount, fdata->fd, true)) != 0) {
    std::string errs(cpp_strerror(r));
-    HT_ERRORF("flush failed: fd=%d  ceph_fd=%d - %s", fd, fdata->fd, errs.c_str());
+    HT_ERRORF("flush failed: fd=%lu ceph_fd=%d - %s", (Lu)fd, fdata->fd, errs.c_str());


tchaikov · 2018-04-13T15:27:49Z

src/client/hypertable/CephBroker.cc


  if (!m_open_file_map.get(fd, fdata)) {
    char errbuf[32];
-    sprintf(errbuf, "%d", fd);
+    sprintf(errbuf, "%u", fd);


tchaikov · 2018-04-13T15:27:54Z

src/client/hypertable/CephBroker.cc

@@ -409,19 +409,19 @@ int CephBroker::rmdir_recursive(const char *directory) {
 void CephBroker::flush(ResponseCallback *cb, uint32_t fd) {
  OpenFileDataCephPtr fdata;

-  HT_DEBUGF("flush fd=%d", fd);
+  HT_DEBUGF("flush fd=%lu", (Lu)fd);


I can do that, but this needs changes in more than these places since this kind of code is already used a lot in the file.

cbodley · 2018-04-13T15:31:09Z

src/rgw/rgw_website.cc

@@ -94,6 +94,7 @@ bool RGWBucketWebsiteConf::should_redirect(const string& key, const int http_err
 	RGWBWRoutingRule redirect_all_rule;
 	redirect_all_rule.redirect_info.redirect = redirect_all;
 	redirect_all.http_redirect_code = 301;
+	redirect_all_rule.condition = {};


can you please move this fix into the condition itself?

struct RGWBWRoutingRuleCondition { std::string key_prefix_equals; - uint16_t http_error_code_returned_equals; + uint16_t http_error_code_returned_equals = 0;

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

Co-authored-by: Casey Bodley <cbodley@redhat.com> Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

Fix for: [src/compressor/lz4/LZ4Compressor.h:105]: (style) Condition 'r!=(int)compressed_pairs[i].first' is always true Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

Fix for: [src/krbd.cc:549]: (portability) Passing NULL after the last typed argument to a variadic function leads to undefined behaviour. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

dalgaaf · 2018-04-14T06:28:11Z

make check failed ... looks like an issue with the agent:

"Agent went offline during the build"

tchaikov · 2018-04-14T13:26:51Z

i guess the "make check" timed out.

retest this please

dalgaaf · 2018-04-14T22:25:31Z

Seems to work now.

tchaikov · 2018-04-16T14:53:16Z

the failed tests are caused by #21338

tchaikov · 2018-04-16T14:58:44Z

@dalgaaf please note, we use ceph/ceph for "official" branches, like jewel, luminous, mimic, and it's advised to send PRs from our own repos instead.

dalgaaf added 2 commits April 12, 2018 20:33

common/DecayCounter.cc: init variable to silence cppcheck

3b6bb4b

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

krbd.cc: fix uninitialized variable

98917fd

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

tchaikov requested a review from dillaman April 13, 2018 14:58

tchaikov added the rbd label Apr 13, 2018

dillaman reviewed Apr 13, 2018

View reviewed changes

dillaman added core common rgw labels Apr 13, 2018

tchaikov requested changes Apr 13, 2018

View reviewed changes

cbodley reviewed Apr 13, 2018

View reviewed changes

dalgaaf force-pushed the wip-da-SCA-20180329 branch from 48aba5b to eaf4dcf Compare April 13, 2018 22:11

dalgaaf and others added 12 commits April 14, 2018 00:13

msg/async/EventKqueue.cc: fix memory leak due to realloc error

81c98a8

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

rgw/rgw_website.h: fix uninitialized variable

f8038f4

Co-authored-by: Casey Bodley <cbodley@redhat.com> Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

test/fio/fio_ceph_objectstore.cc: fix uninitialized variables

ae6df8b

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

osd/OSD.h: fix format qualifier to use PRIu32

2a72622

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

lz4/LZ4Compressor.h: remove always true check

4da48fe

Fix for: [src/compressor/lz4/LZ4Compressor.h:105]: (style) Condition 'r!=(int)compressed_pairs[i].first' is always true Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

krbd.cc: fix parameter to variadic function

7e83dc6

Fix for: [src/krbd.cc:549]: (portability) Passing NULL after the last typed argument to a variadic function leads to undefined behaviour. Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

CephBroker.cc: fix format qualifier for unsigned int

1de5d14

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

common/str_map.cc: do boundry checks before access

84b8387

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

common/TrackedOp.cc: fix format qualifiers for uint

e4852e2

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

mon/OSDMonitor.cc: fix format qualifiers

0aec576

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

src/rgw: fix some format qualifiers

c074a04

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

test_rgw_admin_log.cc: lenght() returns unsigned

0c478d7

Signed-off-by: Danny Al-Gaaf <danny.al-gaaf@bisect.de>

dalgaaf force-pushed the wip-da-SCA-20180329 branch from eaf4dcf to 0c478d7 Compare April 13, 2018 22:16

tchaikov added the wip-kefu-testing label Apr 15, 2018

tchaikov approved these changes Apr 16, 2018

View reviewed changes

tchaikov merged commit 4f787e8 into master Apr 16, 2018

tchaikov deleted the wip-da-SCA-20180329 branch April 16, 2018 14:56

This was referenced Apr 26, 2018

move dmclock subtree into submodule #21651

Merged

misc: mark constructors as explicit #21637

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for issues found during SCA #21419

Fixes for issues found during SCA #21419

dalgaaf commented Apr 13, 2018

dillaman Apr 13, 2018

dillaman Apr 13, 2018

tchaikov Apr 13, 2018

tchaikov Apr 13, 2018

tchaikov Apr 13, 2018

tchaikov Apr 13, 2018

dalgaaf Apr 13, 2018

tchaikov Apr 13, 2018

tchaikov Apr 13, 2018

tchaikov Apr 13, 2018

tchaikov Apr 13, 2018

dalgaaf Apr 13, 2018

cbodley Apr 13, 2018

dalgaaf commented Apr 14, 2018

tchaikov commented Apr 14, 2018

dalgaaf commented Apr 14, 2018

tchaikov commented Apr 16, 2018

tchaikov commented Apr 16, 2018

Fixes for issues found during SCA #21419

Fixes for issues found during SCA #21419

Conversation

dalgaaf commented Apr 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalgaaf commented Apr 14, 2018

tchaikov commented Apr 14, 2018

dalgaaf commented Apr 14, 2018

tchaikov commented Apr 16, 2018

tchaikov commented Apr 16, 2018