You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@celix.apache.org by GitBox <gi...@apache.org> on 2022/11/13 21:31:29 UTC
[GitHub] [celix] pnoltes opened a new pull request, #448: Fix several resource leaks and locking issues reported by coverity
pnoltes opened a new pull request, #448:
URL: https://github.com/apache/celix/pull/448
This PR should fix several issue found by coverity.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on a diff in pull request #448: Fix several resource leaks and locking issues reported by coverity
Posted by GitBox <gi...@apache.org>.
PengZheng commented on code in PR #448:
URL: https://github.com/apache/celix/pull/448#discussion_r1035513751
##########
bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c:
##########
@@ -571,13 +571,15 @@ int pubsub_tcpHandler_listen(pubsub_tcpHandler_t *handle, char *url) {
if (rc != 0) {
L_ERROR("[TCP Socket] Error listen: %s\n", strerror(errno));
pubsub_tcpHandler_freeEntry(entry);
+ close(fd);
entry = NULL;
}
}
if (rc >= 0) {
rc = pubsub_tcpHandler_makeNonBlocking(handle, fd);
if (rc < 0) {
pubsub_tcpHandler_freeEntry(entry);
+ close(fd);
Review Comment:
I also don't like the locking pattern here. Locking IO is anti-pattern, and should be totally avoided if possible. But that belongs to a separate PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on a diff in pull request #448: Fix several resource leaks and locking issues reported by coverity
Posted by GitBox <gi...@apache.org>.
PengZheng commented on code in PR #448:
URL: https://github.com/apache/celix/pull/448#discussion_r1035503277
##########
bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c:
##########
@@ -571,13 +571,15 @@ int pubsub_tcpHandler_listen(pubsub_tcpHandler_t *handle, char *url) {
if (rc != 0) {
L_ERROR("[TCP Socket] Error listen: %s\n", strerror(errno));
pubsub_tcpHandler_freeEntry(entry);
+ close(fd);
entry = NULL;
}
}
if (rc >= 0) {
rc = pubsub_tcpHandler_makeNonBlocking(handle, fd);
if (rc < 0) {
pubsub_tcpHandler_freeEntry(entry);
+ close(fd);
Review Comment:
My fault.
##########
bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c:
##########
@@ -571,13 +571,15 @@ int pubsub_tcpHandler_listen(pubsub_tcpHandler_t *handle, char *url) {
if (rc != 0) {
L_ERROR("[TCP Socket] Error listen: %s\n", strerror(errno));
pubsub_tcpHandler_freeEntry(entry);
+ close(fd);
Review Comment:
It's my fault. `pubsub_tcpHandler_freeEntry` takes care of the fd.
##########
bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c:
##########
@@ -608,6 +610,8 @@ int pubsub_tcpHandler_listen(pubsub_tcpHandler_t *handle, char *url) {
celixThreadRwlock_unlock(&handle->dbLock);
} else {
L_ERROR("[TCP Socket] Error listen socket cannot bind to %s: %s\n", url ? url : "", strerror(errno));
+ free(pUrl);
Review Comment:
missing `free(sin)`.
Let's put `free(sin); free(pUrl)` right after `entry = pubsub_tcpHandler_createEntry(handle, fd, pUrl, NULL, sin);`, so that we can remove them from both branches.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] pnoltes commented on a diff in pull request #448: Fix several resource leaks and locking issues reported by coverity
Posted by GitBox <gi...@apache.org>.
pnoltes commented on code in PR #448:
URL: https://github.com/apache/celix/pull/448#discussion_r1034767046
##########
bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi.c:
##########
@@ -763,11 +763,9 @@ static celix_status_t remoteServiceAdmin_createEndpointDescription(remote_servic
status = CELIX_ENOMEM;
} else {
(*endpoint)->id = (char*) celix_properties_get(endpointProperties, (char*) OSGI_RSA_ENDPOINT_ID, NULL);
- const char *serviceId = NULL;
- serviceReference_getProperty(reference, (char*) OSGI_FRAMEWORK_SERVICE_ID, &serviceId);
- (*endpoint)->serviceId = strtoull(serviceId, NULL, 0);
+ (*endpoint)->serviceId = serviceReference_getServiceId(reference);
(*endpoint)->frameworkUUID = (char*) celix_properties_get(endpointProperties, (char*) OSGI_RSA_ENDPOINT_FRAMEWORK_UUID, NULL);
- (*endpoint)->service = strndup(interface, 1024*10);
+ (*endpoint)->serviceName = celix_utils_strdup(interface);
Review Comment:
I will revert this to the original statement. I agree that a more reasonable user limit should be used, but I think this is better done in a separate RSA oriented PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on a diff in pull request #448: Fix several resource leaks and locking issues reported by coverity
Posted by GitBox <gi...@apache.org>.
PengZheng commented on code in PR #448:
URL: https://github.com/apache/celix/pull/448#discussion_r1033069784
##########
bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c:
##########
@@ -607,6 +607,7 @@ int pubsub_tcpHandler_listen(pubsub_tcpHandler_t *handle, char *url) {
}
celixThreadRwlock_unlock(&handle->dbLock);
} else {
+ free(pUrl);
Review Comment:
Is fd also leaked? I think fd leakage also happens in the other branch.
##########
bundles/remote_services/rsa_common/src/endpoint_description.c:
##########
@@ -92,7 +92,7 @@ static celix_status_t endpointDescription_verifyLongProperty(celix_properties_t
}
bool endpointDescription_isInvalid(const endpoint_description_t *description) {
Review Comment:
This function is used in many places. I suggest that Xu have a look at this to make sure that it will not break some invariants.
What problem does it cause? Shall we apply for coverity access?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on a diff in pull request #448: Fix several resource leaks and locking issues reported by coverity
Posted by GitBox <gi...@apache.org>.
PengZheng commented on code in PR #448:
URL: https://github.com/apache/celix/pull/448#discussion_r1035510287
##########
bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c:
##########
@@ -571,13 +571,15 @@ int pubsub_tcpHandler_listen(pubsub_tcpHandler_t *handle, char *url) {
if (rc != 0) {
L_ERROR("[TCP Socket] Error listen: %s\n", strerror(errno));
pubsub_tcpHandler_freeEntry(entry);
+ close(fd);
entry = NULL;
}
}
if (rc >= 0) {
rc = pubsub_tcpHandler_makeNonBlocking(handle, fd);
if (rc < 0) {
pubsub_tcpHandler_freeEntry(entry);
+ close(fd);
Review Comment:
If `handle->efd < 0`, then `entry` will also be leaked.
I think in this `if(entry != NULL)` branch, we only need one `pubsub_tcpHandler_freeEntry`:
```C
if ((rc >= 0) && (handle->efd >= 0)) {
} else {
pubsub_tcpHandler_freeEntry(entry);
}
```
Others are superfluous and can be removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] codecov-commenter commented on pull request #448: Fix several resource leaks and locking issues reported by coverity
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #448:
URL: https://github.com/apache/celix/pull/448#issuecomment-1312829490
# [Codecov](https://codecov.io/gh/apache/celix/pull/448?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#448](https://codecov.io/gh/apache/celix/pull/448?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4b0d378) into [master](https://codecov.io/gh/apache/celix/commit/8e9cebe7ea88bc0b425031c37d0948057a75bc92?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8e9cebe) will **increase** coverage by `0.04%`.
> The diff coverage is `92.30%`.
```diff
@@ Coverage Diff @@
## master #448 +/- ##
==========================================
+ Coverage 73.71% 73.75% +0.04%
==========================================
Files 181 181
Lines 28786 28789 +3
==========================================
+ Hits 21219 21234 +15
+ Misses 7567 7555 -12
```
| [Impacted Files](https://codecov.io/gh/apache/celix/pull/448?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...s/pubsub/pubsub\_admin\_tcp/src/pubsub\_tcp\_handler.c](https://codecov.io/gh/apache/celix/pull/448/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnVuZGxlcy9wdWJzdWIvcHVic3ViX2FkbWluX3RjcC9zcmMvcHVic3ViX3RjcF9oYW5kbGVyLmM=) | `78.09% <0.00%> (-0.11%)` | :arrow_down: |
| [bundles/shell/shell/src/Shell.cc](https://codecov.io/gh/apache/celix/pull/448/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnVuZGxlcy9zaGVsbC9zaGVsbC9zcmMvU2hlbGwuY2M=) | `92.30% <100.00%> (ø)` | |
| [bundles/shell/shell/src/lb\_command.c](https://codecov.io/gh/apache/celix/pull/448/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnVuZGxlcy9zaGVsbC9zaGVsbC9zcmMvbGJfY29tbWFuZC5j) | `78.33% <100.00%> (-0.18%)` | :arrow_down: |
| [bundles/shell/shell/src/query\_command.c](https://codecov.io/gh/apache/celix/pull/448/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnVuZGxlcy9zaGVsbC9zaGVsbC9zcmMvcXVlcnlfY29tbWFuZC5j) | `84.12% <100.00%> (-0.13%)` | :arrow_down: |
| [libs/framework/include/celix/Trackers.h](https://codecov.io/gh/apache/celix/pull/448/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy9mcmFtZXdvcmsvaW5jbHVkZS9jZWxpeC9UcmFja2Vycy5o) | `91.00% <100.00%> (+0.03%)` | :arrow_up: |
| [libs/framework/src/celix\_framework\_utils.c](https://codecov.io/gh/apache/celix/pull/448/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy9mcmFtZXdvcmsvc3JjL2NlbGl4X2ZyYW1ld29ya191dGlscy5j) | `95.03% <100.00%> (+0.09%)` | :arrow_up: |
| [...shstreams/api/celix/impl/AbstractPushEventSource.h](https://codecov.io/gh/apache/celix/pull/448/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy9wdXNoc3RyZWFtcy9hcGkvY2VsaXgvaW1wbC9BYnN0cmFjdFB1c2hFdmVudFNvdXJjZS5o) | `91.89% <100.00%> (ø)` | |
| [...sub/pubsub\_admin\_zmq/src/pubsub\_zmq\_topic\_sender.c](https://codecov.io/gh/apache/celix/pull/448/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnVuZGxlcy9wdWJzdWIvcHVic3ViX2FkbWluX3ptcS9zcmMvcHVic3ViX3ptcV90b3BpY19zZW5kZXIuYw==) | `85.65% <0.00%> (+1.26%)` | :arrow_up: |
| [...sub/pubsub\_admin\_tcp/src/pubsub\_tcp\_topic\_sender.c](https://codecov.io/gh/apache/celix/pull/448/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnVuZGxlcy9wdWJzdWIvcHVic3ViX2FkbWluX3RjcC9zcmMvcHVic3ViX3RjcF90b3BpY19zZW5kZXIuYw==) | `85.84% <0.00%> (+1.32%)` | :arrow_up: |
| [libs/utils/src/hash\_map.c](https://codecov.io/gh/apache/celix/pull/448/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy91dGlscy9zcmMvaGFzaF9tYXAuYw==) | `95.22% <0.00%> (+1.96%)` | :arrow_up: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on a diff in pull request #448: Fix several resource leaks and locking issues reported by coverity
Posted by GitBox <gi...@apache.org>.
PengZheng commented on code in PR #448:
URL: https://github.com/apache/celix/pull/448#discussion_r1033152688
##########
bundles/remote_services/rsa_common/src/endpoint_description.c:
##########
@@ -92,7 +92,7 @@ static celix_status_t endpointDescription_verifyLongProperty(celix_properties_t
}
bool endpointDescription_isInvalid(const endpoint_description_t *description) {
Review Comment:
Shall we change type of `description->serviceId` to long?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on a diff in pull request #448: Fix several resource leaks and locking issues reported by coverity
Posted by GitBox <gi...@apache.org>.
PengZheng commented on code in PR #448:
URL: https://github.com/apache/celix/pull/448#discussion_r1035510287
##########
bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c:
##########
@@ -571,13 +571,15 @@ int pubsub_tcpHandler_listen(pubsub_tcpHandler_t *handle, char *url) {
if (rc != 0) {
L_ERROR("[TCP Socket] Error listen: %s\n", strerror(errno));
pubsub_tcpHandler_freeEntry(entry);
+ close(fd);
entry = NULL;
}
}
if (rc >= 0) {
rc = pubsub_tcpHandler_makeNonBlocking(handle, fd);
if (rc < 0) {
pubsub_tcpHandler_freeEntry(entry);
+ close(fd);
Review Comment:
If `handle->efd < 0`, then `entry` will also be leaked.
I think in this `if(entry != NULL)` branch, we only need one `pubsub_tcpHandler_freeEntry`:
```C
if ((rc >= 0) && (handle->efd >= 0)) {
} else {
pubsub_tcpHandler_freeEntry(entry);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] pnoltes merged pull request #448: Fix several resource leaks and locking issues reported by coverity
Posted by GitBox <gi...@apache.org>.
pnoltes merged PR #448:
URL: https://github.com/apache/celix/pull/448
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] pnoltes commented on a diff in pull request #448: Fix several resource leaks and locking issues reported by coverity
Posted by GitBox <gi...@apache.org>.
pnoltes commented on code in PR #448:
URL: https://github.com/apache/celix/pull/448#discussion_r1033901659
##########
bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c:
##########
@@ -607,6 +607,7 @@ int pubsub_tcpHandler_listen(pubsub_tcpHandler_t *handle, char *url) {
}
celixThreadRwlock_unlock(&handle->dbLock);
} else {
+ free(pUrl);
Review Comment:
correct, added close of fd.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] pnoltes commented on a diff in pull request #448: Fix several resource leaks and locking issues reported by coverity
Posted by GitBox <gi...@apache.org>.
pnoltes commented on code in PR #448:
URL: https://github.com/apache/celix/pull/448#discussion_r1033902961
##########
bundles/remote_services/rsa_common/src/endpoint_description.c:
##########
@@ -92,7 +92,7 @@ static celix_status_t endpointDescription_verifyLongProperty(celix_properties_t
}
bool endpointDescription_isInvalid(const endpoint_description_t *description) {
Review Comment:
Updated to serviceId type from unsigned long to long and also renamed the service field name to serviceName.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] pnoltes commented on a diff in pull request #448: Fix several resource leaks and locking issues reported by coverity
Posted by GitBox <gi...@apache.org>.
pnoltes commented on code in PR #448:
URL: https://github.com/apache/celix/pull/448#discussion_r1037402059
##########
bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c:
##########
@@ -571,13 +571,15 @@ int pubsub_tcpHandler_listen(pubsub_tcpHandler_t *handle, char *url) {
if (rc != 0) {
L_ERROR("[TCP Socket] Error listen: %s\n", strerror(errno));
pubsub_tcpHandler_freeEntry(entry);
+ close(fd);
entry = NULL;
}
}
if (rc >= 0) {
rc = pubsub_tcpHandler_makeNonBlocking(handle, fd);
if (rc < 0) {
pubsub_tcpHandler_freeEntry(entry);
+ close(fd);
Review Comment:
I agree, good points but something for a separate PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on a diff in pull request #448: Fix several resource leaks and locking issues reported by coverity
Posted by GitBox <gi...@apache.org>.
PengZheng commented on code in PR #448:
URL: https://github.com/apache/celix/pull/448#discussion_r1034269276
##########
bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c:
##########
@@ -607,6 +607,7 @@ int pubsub_tcpHandler_listen(pubsub_tcpHandler_t *handle, char *url) {
}
celixThreadRwlock_unlock(&handle->dbLock);
} else {
+ free(pUrl);
Review Comment:
Then the fd leakage in `else` branch is fixed, while the leakage in the `if` branch remains, e.g., when `rc = listen(fd, SOMAXCONN);` failed. It's OK to leave it there, and adding a `FIXME` reminder would be nice.
I have improved the error injection technique mentioned in #393 so that it can be deployed easily in CMake projects.
To be honest, the one in #393 is ugly and cumbersome to use. Once we have such mechanism in place, emulating socket listening error is fairly easy, and code coverage will not be a problem. I plan to upstream it when working on #441.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on a diff in pull request #448: Fix several resource leaks and locking issues reported by coverity
Posted by GitBox <gi...@apache.org>.
PengZheng commented on code in PR #448:
URL: https://github.com/apache/celix/pull/448#discussion_r1034269276
##########
bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c:
##########
@@ -607,6 +607,7 @@ int pubsub_tcpHandler_listen(pubsub_tcpHandler_t *handle, char *url) {
}
celixThreadRwlock_unlock(&handle->dbLock);
} else {
+ free(pUrl);
Review Comment:
~~Then the fd leakage in `else` branch is fixed, while the leakage in the `if` branch remains, e.g., when `rc = listen(fd, SOMAXCONN);` failed. It's OK to leave it there, and adding a `FIXME` reminder would be nice.~~
I have improved the error injection technique mentioned in #393 so that it can be deployed easily in CMake projects.
To be honest, the one in #393 is ugly and cumbersome to use. Once we have such mechanism in place, emulating socket listening error is fairly easy, and code coverage will not be a problem. I plan to upstream it when working on #441.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on a diff in pull request #448: Fix several resource leaks and locking issues reported by coverity
Posted by GitBox <gi...@apache.org>.
PengZheng commented on code in PR #448:
URL: https://github.com/apache/celix/pull/448#discussion_r1034269276
##########
bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c:
##########
@@ -607,6 +607,7 @@ int pubsub_tcpHandler_listen(pubsub_tcpHandler_t *handle, char *url) {
}
celixThreadRwlock_unlock(&handle->dbLock);
} else {
+ free(pUrl);
Review Comment:
Then the fd leakage in `else` branch is fixed, while the leakage in the `if` branch remains, e.g., when `rc = listen(fd, SOMAXCONN);` failed.
I have improved the error injection technique mentioned in #393 so that it can be deployed easily in CMake projects.
To be honest, the one in #393 is ugly and cumbersome to use. Once we have such mechanism in place, emulating socket listening error is fairly easy, and code coverage will not be a problem. I plan to upstream it when working on #441.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on a diff in pull request #448: Fix several resource leaks and locking issues reported by coverity
Posted by GitBox <gi...@apache.org>.
PengZheng commented on code in PR #448:
URL: https://github.com/apache/celix/pull/448#discussion_r1034269276
##########
bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c:
##########
@@ -607,6 +607,7 @@ int pubsub_tcpHandler_listen(pubsub_tcpHandler_t *handle, char *url) {
}
celixThreadRwlock_unlock(&handle->dbLock);
} else {
+ free(pUrl);
Review Comment:
Then the fd leakage in `else` branch is fixed, while the leakage in the `if` branch remains, e.g., when `rc = listen(fd, SOMAXCONN);` failed.
I have improved the error injection technique mentioned in #399 so that it can be deployed easily in CMake projects.
To be honest, the one in #399 is ugly and cumbersome to use. Once we have such mechanism in place, emulating socket listening error is fairly easy, and code coverage will not be a problem. I plan to upstream it when working on #441.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] PengZheng commented on a diff in pull request #448: Fix several resource leaks and locking issues reported by coverity
Posted by GitBox <gi...@apache.org>.
PengZheng commented on code in PR #448:
URL: https://github.com/apache/celix/pull/448#discussion_r1034279410
##########
bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi.c:
##########
@@ -763,11 +763,9 @@ static celix_status_t remoteServiceAdmin_createEndpointDescription(remote_servic
status = CELIX_ENOMEM;
} else {
(*endpoint)->id = (char*) celix_properties_get(endpointProperties, (char*) OSGI_RSA_ENDPOINT_ID, NULL);
- const char *serviceId = NULL;
- serviceReference_getProperty(reference, (char*) OSGI_FRAMEWORK_SERVICE_ID, &serviceId);
- (*endpoint)->serviceId = strtoull(serviceId, NULL, 0);
+ (*endpoint)->serviceId = serviceReference_getServiceId(reference);
(*endpoint)->frameworkUUID = (char*) celix_properties_get(endpointProperties, (char*) OSGI_RSA_ENDPOINT_FRAMEWORK_UUID, NULL);
- (*endpoint)->service = strndup(interface, 1024*10);
+ (*endpoint)->serviceName = celix_utils_strdup(interface);
Review Comment:
`celix_utils_strdup` has a very large internal upper limit for string length. It may not be a problem here, since it comes from `celix_properties` in the framework.
But when importing external properties into the Celix framework, we must set a more reasonable upper limit . This is especially true if we are implementing something like DNS-SD in Celix. @xuzhenbao
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org