Skip to content

Fix Coverity issue in inliner plugin. #10442

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.

Sign up for GitHub

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

Merged
ywkaras merged 2 commits into from
Sep 19, 2023
Merged

Fix Coverity issue in inliner plugin. #10442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5783644
Fix Coverity issue in inliner plugin.
ywkaras Sep 14, 2023
4dfdc04
Review changes.
ywkaras Sep 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions plugins/experimental/inliner/cache-handler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,11 +326,12 @@ namespace inliner
request += std::string(b1, i);
request += "\r\n\r\n";

ats::io::IO *const io = new io::IO();
auto io{std::make_unique<io::IO>()};

TSDebug(PLUGIN_TAG, "request:\n%s", request.c_str());
Copy link
Copy Markdown
Contributor Author

@ywkaras ywkaras Sep 15, 2023

Choose a reason for hiding this comment

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

It's not clear what exactly Coverity's gripe is. My best guess is that it's seeing that *io would get leaked if TSDebug() threw an exception. But there must be a bazillion cases of that in our code, why would it single this one out? Are there instructions anywhere about how to run Coverity, to be sure we've appeased it?

Copy link
Copy Markdown
Contributor

@zwoop zwoop Sep 17, 2023

Choose a reason for hiding this comment

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

No unfortunately we don’t have enough karma to run PRs against coverity. So it’ll have to land on master for the next schedule run, rinse repeats…

Copy link
Copy Markdown
Contributor

@cmcfarlen cmcfarlen Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It looks like from reading the defect report, coverity doesn't trace into the second get call to see that it is indeed assigned eventually. The std::move will hopefully help it realize that ownership is handed off.


ats::get(io, io->copy(request), AnotherClass(src_));
auto size{io->copy(request)};
ats::get(std::move(io), size, AnotherClass(src_));
}
};

Expand Down
25 changes: 11 additions & 14 deletions plugins/experimental/inliner/fetcher.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@
#include <netinet/in.h>

#include <cinttypes>
#include <memory>
#include <utility>

#include "chunk-decoder.h"
#include "ts.h"
Expand Down Expand Up @@ -98,7 +100,7 @@ template <class T> struct HttpTransaction {
bool abort_;
bool timeout_;
io::IO *in_;
io::IO *out_;
std::unique_ptr<io::IO> out_;
TSVConn vconnection_;
TSCont continuation_;
T t_;
Expand All @@ -111,10 +113,6 @@ template <class T> struct HttpTransaction {
delete in_;
in_ = nullptr;
}
if (out_ != nullptr) {
delete out_;
out_ = nullptr;
}
timeout(0);
assert(vconnection_ != nullptr);
if (abort_) {
Expand All @@ -129,20 +127,20 @@ template <class T> struct HttpTransaction {
}
}

HttpTransaction(TSVConn v, TSCont c, io::IO *const i, const uint64_t l, const T &t)
HttpTransaction(TSVConn v, TSCont c, std::unique_ptr<io::IO> i, const uint64_t l, const T &t)
: parsingHeaders_(false),
abort_(false),
timeout_(false),
in_(nullptr),
out_(i),
out_(std::move(i)),
vconnection_(v),
continuation_(c),
t_(t),
chunkDecoder_(nullptr)
{
assert(vconnection_ != nullptr);
assert(continuation_ != nullptr);
assert(out_ != nullptr);
assert(out_.get() != nullptr);
assert(l > 0);
out_->vio = TSVConnWrite(vconnection_, continuation_, out_->reader, l);
}
Expand Down Expand Up @@ -271,8 +269,7 @@ template <class T> struct HttpTransaction {
assert(self->vconnection_);
TSVConnShutdown(self->vconnection_, 0, 1);
assert(self->out_ != nullptr);
delete self->out_;
self->out_ = nullptr;
self->out_.reset();
break;
case TS_EVENT_VCONN_WRITE_READY:
TSDebug(PLUGIN_TAG, "HttpTransaction: Write Ready (Done: %" PRId64 " Todo: %" PRId64 ")", TSVIONDoneGet(self->out_->vio),
Expand All @@ -299,7 +296,7 @@ template <class T> struct HttpTransaction {

template <class T>
bool
get(const std::string &a, io::IO *const i, const int64_t l, const T &t, const int64_t ti = 0)
get(const std::string &a, std::unique_ptr<io::IO> i, const int64_t l, const T &t, const int64_t ti = 0)
{
using Transaction = HttpTransaction<T>;
struct sockaddr_in socket;
Expand All @@ -313,7 +310,7 @@ get(const std::string &a, io::IO *const i, const int64_t l, const T &t, const in
assert(vconn != nullptr);
TSCont contp = TSContCreate(Transaction::handle, nullptr);
assert(contp != nullptr);
Transaction *transaction = new Transaction(vconn, contp, i, l, t);
Transaction *transaction = new Transaction(vconn, contp, std::move(i), l, t);
TSContDataSet(contp, transaction);
if (ti > 0) {
TSDebug(PLUGIN_TAG, "ats::get Setting active timeout to: %" PRId64, ti);
Expand All @@ -324,8 +321,8 @@ get(const std::string &a, io::IO *const i, const int64_t l, const T &t, const in

template <class T>
bool
get(io::IO *const i, const int64_t l, const T &t, const int64_t ti = 0)
get(std::unique_ptr<io::IO> i, const int64_t l, const T &t, const int64_t ti = 0)
{
return get("127.0.0.1", i, l, t, ti);
return get("127.0.0.1", std::move(i), l, t, ti);
}
} // namespace ats