You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by strotyl <gi...@git.apache.org> on 2016/06/24 16:17:09 UTC
[GitHub] trafficserver pull request #738: TS-4577 remap_stats: Fix memory leak
GitHub user strotyl opened a pull request:
https://github.com/apache/trafficserver/pull/738
TS-4577 remap_stats: Fix memory leak
This is fixing CID 1356995 & 1356996.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/strotyl/trafficserver TS-4577
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/738.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #738
----
commit b643bec071c3193e7d75fe24c99ef74796be138d
Author: Tyler Stroh <ts...@apple.com>
Date: 2016-06-24T14:29:03Z
TS-4577 remap_stats: Fix memory leak
This is fixing CID 1356995 & 1356996.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #738: TS-4577 remap_stats: Fix memory leak
Posted by PSUdaemon <gi...@git.apache.org>.
Github user PSUdaemon commented on the issue:
https://github.com/apache/trafficserver/pull/738
[approve ci]
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #738: TS-4577 remap_stats: Fix memory leak
Posted by PSUdaemon <gi...@git.apache.org>.
Github user PSUdaemon commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/738#discussion_r68441459
--- Diff: plugins/experimental/remap_stats/remap_stats.c ---
@@ -92,19 +92,20 @@ stat_add(char *name, TSMgmtInt amount, TSStatPersistence persist_type, TSMutex c
static char *
get_effective_host(TSHttpTxn txn)
{
- char *effective_url, *tmp;
const char *host;
int len;
TSMBuffer buf;
TSMLoc url_loc;
+ char *effective_url, *tmp;
- effective_url = TSHttpTxnEffectiveUrlStringGet(txn, &len);
- buf = TSMBufferCreate();
+ buf = TSMBufferCreate();
if (TS_SUCCESS != TSUrlCreate(buf, &url_loc)) {
TSDebug(DEBUG_TAG, "unable to create url");
+ TSMBufferDestroy(buf);
return NULL;
}
- tmp = effective_url;
+
--- End diff --
Was this space addition intentional?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #738: TS-4577 remap_stats: Fix memory leak
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/738
[approve ci]
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #738: TS-4577 remap_stats: Fix memory leak
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/738
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/232/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #738: TS-4577 remap_stats: Fix memory leak
Posted by strotyl <gi...@git.apache.org>.
Github user strotyl commented on the issue:
https://github.com/apache/trafficserver/pull/738
Ok, I've pushed another update moving the stack allocations back to the top of the function.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #738: TS-4577 remap_stats: Fix memory leak
Posted by strotyl <gi...@git.apache.org>.
Github user strotyl commented on the issue:
https://github.com/apache/trafficserver/pull/738
I just saw your note... I'm building another update.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #738: TS-4577 remap_stats: Fix memory leak
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/738
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/336/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #738: TS-4577 remap_stats: Fix memory leak
Posted by PSUdaemon <gi...@git.apache.org>.
Github user PSUdaemon commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/738#discussion_r68441181
--- Diff: plugins/experimental/remap_stats/remap_stats.c ---
@@ -92,19 +92,20 @@ stat_add(char *name, TSMgmtInt amount, TSStatPersistence persist_type, TSMutex c
static char *
get_effective_host(TSHttpTxn txn)
{
- char *effective_url, *tmp;
const char *host;
int len;
TSMBuffer buf;
TSMLoc url_loc;
+ char *effective_url, *tmp;
--- End diff --
Why has this changed places? I don't think we should introduce changes that don't make things better. This is effectively a net zero change and I don't think we should make it. It just makes diff's harder to read.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #738: TS-4577 remap_stats: Fix memory leak
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/738
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/233/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #738: TS-4577 remap_stats: Fix memory leak
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/738
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/337/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #738: TS-4577 remap_stats: Fix memory leak
Posted by PSUdaemon <gi...@git.apache.org>.
Github user PSUdaemon closed the pull request at:
https://github.com/apache/trafficserver/pull/738
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #738: TS-4577 remap_stats: Fix memory leak
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/738
[approve ci]
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #738: TS-4577 remap_stats: Fix memory leak
Posted by strotyl <gi...@git.apache.org>.
Github user strotyl commented on the issue:
https://github.com/apache/trafficserver/pull/738
Thats a good idea. I'll adjust my code and resubmit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #738: TS-4577 remap_stats: Fix memory leak
Posted by PSUdaemon <gi...@git.apache.org>.
Github user PSUdaemon commented on the issue:
https://github.com/apache/trafficserver/pull/738
What do you think of moving the `effective_url` creation down below so that we don't even alloc it until we are past that check?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #738: TS-4577 remap_stats: Fix memory leak
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/738
[approve ci]
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #738: TS-4577 remap_stats: Fix memory leak
Posted by strotyl <gi...@git.apache.org>.
Github user strotyl commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/738#discussion_r68444862
--- Diff: plugins/experimental/remap_stats/remap_stats.c ---
@@ -92,19 +92,20 @@ stat_add(char *name, TSMgmtInt amount, TSStatPersistence persist_type, TSMutex c
static char *
get_effective_host(TSHttpTxn txn)
{
- char *effective_url, *tmp;
const char *host;
int len;
TSMBuffer buf;
TSMLoc url_loc;
+ char *effective_url, *tmp;
--- End diff --
Understood. I'll move it back up the top.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #738: TS-4577 remap_stats: Fix memory leak
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/738
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/338/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #738: TS-4577 remap_stats: Fix memory leak
Posted by PSUdaemon <gi...@git.apache.org>.
Github user PSUdaemon commented on the issue:
https://github.com/apache/trafficserver/pull/738
Just for the record, I was thinking: `tmp = effective_url = TSHttpTxnEffectiveUrlStringGet(txn, &len);`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #738: TS-4577 remap_stats: Fix memory leak
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/738
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/339/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #738: TS-4577 remap_stats: Fix memory leak
Posted by PSUdaemon <gi...@git.apache.org>.
Github user PSUdaemon commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/738#discussion_r68431089
--- Diff: plugins/experimental/remap_stats/remap_stats.c ---
@@ -92,19 +92,19 @@ stat_add(char *name, TSMgmtInt amount, TSStatPersistence persist_type, TSMutex c
static char *
get_effective_host(TSHttpTxn txn)
{
- char *effective_url, *tmp;
const char *host;
int len;
TSMBuffer buf;
TSMLoc url_loc;
- effective_url = TSHttpTxnEffectiveUrlStringGet(txn, &len);
- buf = TSMBufferCreate();
+ buf = TSMBufferCreate();
if (TS_SUCCESS != TSUrlCreate(buf, &url_loc)) {
TSDebug(DEBUG_TAG, "unable to create url");
+ TSMBufferDestroy(buf);
return NULL;
}
- tmp = effective_url;
+ char *effective_url, *tmp;
--- End diff --
Why are we moving this down here? Since it's stack allocated the compiler won't care that you put it after the `if` above. Really all we want to avoid is making the call to `TSHttpTxnEffectiveUrlStringGet` which will heap alloc the memory pointed to by `effective_url`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #738: TS-4577 remap_stats: Fix memory leak
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/738
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/230/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #738: TS-4577 remap_stats: Fix memory leak
Posted by strotyl <gi...@git.apache.org>.
Github user strotyl commented on the issue:
https://github.com/apache/trafficserver/pull/738
OK, Updated. Ready for build. :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #738: TS-4577 remap_stats: Fix memory leak
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/738
Linux build *failed*! See https://ci.trafficserver.apache.org/job/Github-Linux/231/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---