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

rgw: civetweb don't go past the array index while calling mg_start #14750

Merged
merged 1 commit into from May 1, 2017

Conversation

Projects
None yet
4 participants
@theanalyst
Copy link
Member

theanalyst commented Apr 24, 2017

currently we set the array size as the members in conf_map minus the
members in rgw_opts, however all of the options inside rgw_opts like
prefix do not feature in the conf_map hence the array is bigger than
the expected size, and causes undefined behaviour depending on the
compiler flags used.

Fixes: http://tracker.ceph.com/issues/19749
Signed-off-by: Abhishek Lekshmanan abhishek@suse.com

@theanalyst

This comment has been minimized.

Copy link
Member Author

theanalyst commented Apr 24, 2017

Another option would be to just use a vector<const char*> for options and just pass the vector.data() to mg_start

@rzarzynski rzarzynski self-assigned this Apr 25, 2017

if (conf_map.find(opt) != conf_map.end())
rgw_opt_num++;
}
const size_t CW_NUM_OPTS = 2 * (conf_map.size() - rgw_opt_num) + 1;
const char *options[CW_NUM_OPTS];

This comment has been minimized.

Copy link
@chardan

chardan Apr 25, 2017

Contributor

I'm very surprised this is compiling-- I'd expect at least a warning. C++ doesn't support variable-length arrays, and conf_map isn't a constant... (Are we using GNU extensions?) I suggest std::vector<>.
https://gcc.gnu.org/onlinedocs/gcc/Variable-Length.html
https://groups.google.com/forum/#!topic/comp.std.c++/K_4lgA1JYeg

@theanalyst

This comment has been minimized.

Copy link
Member Author

theanalyst commented Apr 25, 2017

Pushed theanalyst@426564d which implements options as a vector, this looks better than the current pr IMO

edit: upated ref after rebase

@theanalyst

This comment has been minimized.

Copy link
Member Author

theanalyst commented Apr 26, 2017

@rzarzynski can you take a look and suggest which of the approaches would be better?

@rzarzynski

This comment has been minimized.

Copy link
Contributor

rzarzynski commented Apr 26, 2017

@theanalyst: thanks for the patches. I would say the vector-based version is nicer, definitely easier to read. I'm voting for it!

I left some comments in theanalyst@426564d.

@theanalyst theanalyst force-pushed the theanalyst:rgw/fix/frontend_array branch from 14beda1 to 07fff3e Apr 26, 2017

@theanalyst

This comment has been minimized.

Copy link
Member Author

theanalyst commented Apr 26, 2017

@rzarzynski thanks for the review, updated this pr to the vector one, addressed the comments

@rzarzynski
Copy link
Contributor

rzarzynski left a comment

@theanalyst: thanks! I'm taking the patch into testing. I will schedule a Teuthology run soon.

const char *options[CW_NUM_OPTS];
size_t i = 0;

std::vector <const char*> options;

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Apr 26, 2017

Contributor

May we also drop the space before <?

This comment has been minimized.

Copy link
@theanalyst

theanalyst Apr 26, 2017

Author Member

ack

@rzarzynski

This comment has been minimized.

Copy link
Contributor

rzarzynski commented Apr 26, 2017

@theanalyst: one more thing. The commit message lacks the Fixes: http://tracker.ceph.com/issues/19749 line. Could you please update?

Sorry for pointing out this lately.

@theanalyst theanalyst force-pushed the theanalyst:rgw/fix/frontend_array branch from 07fff3e to 1a81aee Apr 26, 2017

rgw: use a vector for options passed to civetweb
Since the array we used needs additional check to ensure that the size
is correct, and causes undefined behaviour in a few platforms, using a
vector and passing the c array back to mg_start so that we don't go past
the end of array.

Fixes: http://tracker.ceph.com/issues/19749
Signed-off-by: Abhishek Lekshmanan <abhishek@suse.com>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>

@theanalyst theanalyst force-pushed the theanalyst:rgw/fix/frontend_array branch from 1a81aee to 3959a8b Apr 26, 2017

@theanalyst

This comment has been minimized.

Copy link
Member Author

theanalyst commented Apr 26, 2017

@rzarzynski yeah missed the tracker message when I updated the pr, fixed, thanks!

@rzarzynski

This comment has been minimized.

Copy link
Contributor

rzarzynski commented Apr 26, 2017

Thanks, Abhishek. I've just pushed a branch with the commit for gitbuilders.

@rzarzynski

This comment has been minimized.

Copy link
Contributor

rzarzynski commented Apr 26, 2017

One of the builds failed due to an unrelated failure: https://shaman.ceph.com/builds/ceph/wip-rzarzynski-testing-2/2fff2ab4442b347b315416957e682687c9d42af8/. Restarting.

@rzarzynski

This comment has been minimized.

Copy link
Contributor

rzarzynski commented Apr 27, 2017

@theanalyst: I wasn't able to schedule a Teuthology run today. :-( The teuthology-suite throws at me:

IOError: /home/rzarzynski/src/git.ceph.com_ceph-c_wip-rzarzynski-testing-2/qa/suites/rgw/singleton/xfs.yaml does not exist (abs /home/rzarzynski/src/git.ceph.com_ceph-c_wip-rzarzynski-testing-2/qa/suites/rgw/singleton/xfs.yaml)

It might be that we're experiencing issues with CI in general. shaman.ceph.com is loaded is very odd entries like $ghprbActualCommit. I will try again in the morning.

@theanalyst

This comment has been minimized.

Copy link
Member Author

theanalyst commented Apr 27, 2017

@rzarzynski np, thanks for taking this on.

@rzarzynski

This comment has been minimized.

Copy link
Contributor

rzarzynski commented Apr 27, 2017

Still the same. Repushed the branch once again.

@rzarzynski

This comment has been minimized.

Copy link
Contributor

rzarzynski commented Apr 28, 2017

@rzarzynski

This comment has been minimized.

Copy link
Contributor

rzarzynski commented May 1, 2017

This branch has been tested in following Teuthology runs:

The results look good. The failures have been caused by the well-known strange leak of std::string memory from md_config_t seen in radosgw signalised by Valgrind.

@rzarzynski rzarzynski merged commit b4099a9 into ceph:master May 1, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
arm build successfully built on arm
Details
default Build finished.
Details
@cbodley

This comment has been minimized.

Copy link
Contributor

cbodley commented May 1, 2017

@rzarzynski we suppress those std::string leaks, so they don't result in failures. the valgrind failures i saw from that run were in the mons:

2017-04-30T13:14:48.176 INFO:teuthology.orchestra.run.mira061.stdout:/var/log/ceph/valgrind/mon.b.log:  <kind>Leak_StillReachable</kind>
2017-04-30T13:14:48.193 INFO:teuthology.orchestra.run.mira085.stdout:/var/log/ceph/valgrind/mon.a.log:  <kind>Leak_StillReachable</kind>
2017-04-30T13:14:48.193 INFO:teuthology.orchestra.run.mira085.stdout:/var/log/ceph/valgrind/mon.c.log:  <kind>Leak_StillReachable</kind>
@rzarzynski

This comment has been minimized.

Copy link
Contributor

rzarzynski commented May 1, 2017

@cbodley: ah, I haven't noticed that e.g. client0.log.gz contains

  <pair>
    <count>1</count>
    <name>ignore libfcgi leak; OS_LibShutdown has no callers!</name>
  </pair>
  <pair>
    <count>4</count>
    <name>strange leak of std::string memory from md_config_t seen in radosgw</name>
  </pair>

in <suppcounts>, not <errorcounts>. My bad. Thanks for pointing this out.

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.