You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2021/03/17 02:06:27 UTC
[GitHub] [qpid-dispatch] ganeshmurthy opened a new pull request #1075: DISPATCH-1660: Introduced a hash_handle on the item so we can set the…
ganeshmurthy opened a new pull request #1075:
URL: https://github.com/apache/qpid-dispatch/pull/1075
… item->hash_handle->item to zero when the item is freed. This ensures that the hash_handle will never be able to reach the freed item
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] asfgit closed pull request #1075: DISPATCH-1660: Introduced a hash_handle on the item so we can set the…
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1075:
URL: https://github.com/apache/qpid-dispatch/pull/1075
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #1075: DISPATCH-1660: Introduced a hash_handle on the item so we can set the…
Posted by GitBox <gi...@apache.org>.
ChugR commented on a change in pull request #1075:
URL: https://github.com/apache/qpid-dispatch/pull/1075#discussion_r596001818
##########
File path: src/hash.c
##########
@@ -174,6 +194,15 @@ static qd_hash_item_t *qd_hash_internal_insert(qd_hash_t *h, bucket_t *bucket, u
*handle = new_qd_hash_handle_t();
(*handle)->bucket = bucket;
(*handle)->item = item;
+
+ //
+ // There is ever only one hash_handle that points to an item.
+ // We will store than hash_handle in the item itself because
Review comment:
s/than/that/
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] kgiusti commented on a change in pull request #1075: DISPATCH-1660: Introduced a hash_handle on the item so we can set the…
Posted by GitBox <gi...@apache.org>.
kgiusti commented on a change in pull request #1075:
URL: https://github.com/apache/qpid-dispatch/pull/1075#discussion_r596228606
##########
File path: src/hash.c
##########
@@ -105,11 +106,24 @@ qd_hash_t *qd_hash(int bucket_exponent, int batch_size, int value_is_const)
//remove the given item from the given bucket of the given hash
//return the key if non-null key pointer given, otherwise, free the memory
static void qd_hash_internal_remove_item(qd_hash_t *h, bucket_t *bucket, qd_hash_item_t *item, unsigned char **key) {
- if (key)
+ if (key) {
*key = item->key;
- else
+ }
+ else {
free(item->key);
+ item->key = 0;
+ }
DEQ_REMOVE(bucket->items, item);
+
+ //
+ // We are going to free this item, so we will set the
+ // item pointer on the hash_handle to zero so nobody with
+ // access to the hash_handle can ever try to get to this freed item.
+ // The item and the hash_handle can be freed independent of one another.
+ //
+ if (item->handle) {
Review comment:
Why not make the handle->item a safe pointer to an item? Then you wouldn't have to do this storing/clearing of the handle pointer in the item itself.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] ganeshmurthy commented on a change in pull request #1075: DISPATCH-1660: Introduced a hash_handle on the item so we can set the…
Posted by GitBox <gi...@apache.org>.
ganeshmurthy commented on a change in pull request #1075:
URL: https://github.com/apache/qpid-dispatch/pull/1075#discussion_r596233344
##########
File path: src/hash.c
##########
@@ -105,11 +106,24 @@ qd_hash_t *qd_hash(int bucket_exponent, int batch_size, int value_is_const)
//remove the given item from the given bucket of the given hash
//return the key if non-null key pointer given, otherwise, free the memory
static void qd_hash_internal_remove_item(qd_hash_t *h, bucket_t *bucket, qd_hash_item_t *item, unsigned char **key) {
- if (key)
+ if (key) {
*key = item->key;
- else
+ }
+ else {
free(item->key);
+ item->key = 0;
+ }
DEQ_REMOVE(bucket->items, item);
+
+ //
+ // We are going to free this item, so we will set the
+ // item pointer on the hash_handle to zero so nobody with
+ // access to the hash_handle can ever try to get to this freed item.
+ // The item and the hash_handle can be freed independent of one another.
+ //
+ if (item->handle) {
Review comment:
I thought about taking the safe pointer route but in this case there is ever only one hash_handle per item, so this seemed like the easily understandable approach.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] kgiusti commented on a change in pull request #1075: DISPATCH-1660: Introduced a hash_handle on the item so we can set the…
Posted by GitBox <gi...@apache.org>.
kgiusti commented on a change in pull request #1075:
URL: https://github.com/apache/qpid-dispatch/pull/1075#discussion_r596377973
##########
File path: src/hash.c
##########
@@ -105,11 +106,24 @@ qd_hash_t *qd_hash(int bucket_exponent, int batch_size, int value_is_const)
//remove the given item from the given bucket of the given hash
//return the key if non-null key pointer given, otherwise, free the memory
static void qd_hash_internal_remove_item(qd_hash_t *h, bucket_t *bucket, qd_hash_item_t *item, unsigned char **key) {
- if (key)
+ if (key) {
*key = item->key;
- else
+ }
+ else {
free(item->key);
+ item->key = 0;
+ }
DEQ_REMOVE(bucket->items, item);
+
+ //
+ // We are going to free this item, so we will set the
+ // item pointer on the hash_handle to zero so nobody with
+ // access to the hash_handle can ever try to get to this freed item.
+ // The item and the hash_handle can be freed independent of one another.
+ //
+ if (item->handle) {
Review comment:
Seems reasonable to me!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] ganeshmurthy commented on a change in pull request #1075: DISPATCH-1660: Introduced a hash_handle on the item so we can set the…
Posted by GitBox <gi...@apache.org>.
ganeshmurthy commented on a change in pull request #1075:
URL: https://github.com/apache/qpid-dispatch/pull/1075#discussion_r596233344
##########
File path: src/hash.c
##########
@@ -105,11 +106,24 @@ qd_hash_t *qd_hash(int bucket_exponent, int batch_size, int value_is_const)
//remove the given item from the given bucket of the given hash
//return the key if non-null key pointer given, otherwise, free the memory
static void qd_hash_internal_remove_item(qd_hash_t *h, bucket_t *bucket, qd_hash_item_t *item, unsigned char **key) {
- if (key)
+ if (key) {
*key = item->key;
- else
+ }
+ else {
free(item->key);
+ item->key = 0;
+ }
DEQ_REMOVE(bucket->items, item);
+
+ //
+ // We are going to free this item, so we will set the
+ // item pointer on the hash_handle to zero so nobody with
+ // access to the hash_handle can ever try to get to this freed item.
+ // The item and the hash_handle can be freed independent of one another.
+ //
+ if (item->handle) {
Review comment:
I thought about taking the safe pointer route but in this case there is every only one hash_handle per item, so this seemed like the easily understandable approach.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org