You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2021/12/08 02:41:50 UTC
[GitHub] [trafficserver] bneradt opened a new pull request #8550: TSUserArg: add value type checking
bneradt opened a new pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550
A plugin may have multiple user argument types. For instance, it may
have both TS_USER_ARGS_SSN and TS_USER_ARGS_TXN plugin data. Due to a
coding error, it is possible that it may accidentally use the wrong
index type while retrieving one or the other user data types. For
instance, if it uses its transaction index to retrieve its session data,
this may result in them getting the transaction data for another plugin.
In these situations, the plugin will either read what looks like garbage
data to it (because it is casting incorrect memory to its data type), or
it may write to that data and thus corrupt the other plugin's data, or
both.
This change updates the core to segment off the plugin user data ids by
category. This allows the core to verify that the plugin passes the
correct id type per the data type (session, transaction, etc.) and
fails an assertion if there is a mismatch.
---
Note: these tests will fail until #8548 is merged in.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] duke8253 commented on pull request #8550: TSUserArg: add value type checking
Posted by GitBox <gi...@apache.org>.
duke8253 commented on pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550#issuecomment-990058346
I like this
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt removed a comment on pull request #8550: TSUserArg: add value type checking
Posted by GitBox <gi...@apache.org>.
bneradt removed a comment on pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550#issuecomment-993110687
[approve ci centos]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #8550: TSUserArg: add value type checking
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550#issuecomment-993110687
[approve ci centos]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #8550: TSUserArg: add value type checking
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550#issuecomment-993096830
[approve ci centos]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #8550: TSUserArg: add value type checking
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550#discussion_r765226892
##########
File path: include/tscore/PluginUserArgs.h
##########
@@ -35,6 +35,40 @@ static constexpr std::array<size_t, TS_USER_ARGS_COUNT> MAX_USER_ARGS = {{
128 /* max number of user arguments for GLB */
}};
+/** Stagger each user argument value so we can detect mismatched
+ * indices.
+ *
+ * For example, say a plugin associates data with both sessions and
+ * transactions and that its session index is 2 and its transaction index is 4.
+ * In this case, we'll hand back to the plugin 2002 for its session index and
+ * 1004 for its transaction index. If it then accidentally uses its session
+ * index to reference its transaction index, it will pass back 2002 instead of
+ * 1004, which we will identify as belonging to the wrong user argument type
+ * because it is in the 2000 session block rather than the expected 1000
+ * transaction block.
+ *
+ * Note that these higher value 1000 block indices are only used when
+ * interfacing with the plugin. Internally the lower valued index is used.
+ * That is, for a transaction, expect internally a value of 3 instead of 1003.
+ */
+static constexpr std::array<size_t, TS_USER_ARGS_COUNT> USER_ARGS_OFFSETS = {{
+ 1000, /* max number of user arguments for TXN */
+ 2000, /* max number of user arguments for SSN */
+ 3000, /* max number of user arguments for VCONN */
+ 4000 /* max number of user arguments for GLB */
+}};
Review comment:
I like this idea.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #8550: TSUserArg: add value type checking
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550#issuecomment-993112646
[approve ci centos]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt removed a comment on pull request #8550: TSUserArg: add value type checking
Posted by GitBox <gi...@apache.org>.
bneradt removed a comment on pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550#issuecomment-993112646
[approve ci centos]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #8550: TSUserArg: add value type checking
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550#issuecomment-1006048712
I'd like to leave this out of 9.1.x if ok with you.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt merged pull request #8550: TSUserArg: add value type checking
Posted by GitBox <gi...@apache.org>.
bneradt merged pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #8550: TSUserArg: add value type checking
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550#issuecomment-993109462
[approve ci centos]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 removed a comment on pull request #8550: TSUserArg: add value type checking
Posted by GitBox <gi...@apache.org>.
masaori335 removed a comment on pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550#issuecomment-993087481
[approve ci centos]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt removed a comment on pull request #8550: TSUserArg: add value type checking
Posted by GitBox <gi...@apache.org>.
bneradt removed a comment on pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550#issuecomment-993096830
[approve ci centos]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #8550: TSUserArg: add value type checking
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550#issuecomment-993026422
@masaori335 has volunteered to review this.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #8550: TSUserArg: add value type checking
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550#issuecomment-1006048992
Cherry-picked to v9.2.x
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #8550: TSUserArg: add value type checking
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550#issuecomment-990049558
[approve ci autest]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #8550: TSUserArg: add value type checking
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550#issuecomment-993097388
[approve ci centos]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt removed a comment on pull request #8550: TSUserArg: add value type checking
Posted by GitBox <gi...@apache.org>.
bneradt removed a comment on pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550#issuecomment-993097388
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #8550: TSUserArg: add value type checking
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550#issuecomment-988989720
Nitpick: seems like static constexpr items should be private members of PluginUserArgsMixin. But that's perhaps beyond the scope of this 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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #8550: TSUserArg: add value type checking
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550#issuecomment-990056215
Unfortunately I think this is probably detecting errors in core plugins, that you'll need to fix in this PR to get Au test regression to pass. I suggest trying the failing cases in your own test environment, see what's going on.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #8550: TSUserArg: add value type checking
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550#issuecomment-990059431
> Unfortunately I think this is probably detecting errors in core plugins, that you'll need to fix in this PR to get Au test regression to pass. I suggest trying the failing cases in your own test environment, see what's going on.
Correct, there were issues with core plugins. But I think those are now both addressed and merged in via the PRs mentioned in the description. We'll see whether I missed any with this latest autest run.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #8550: TSUserArg: add value type checking
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550#discussion_r765024858
##########
File path: include/tscore/PluginUserArgs.h
##########
@@ -35,6 +35,40 @@ static constexpr std::array<size_t, TS_USER_ARGS_COUNT> MAX_USER_ARGS = {{
128 /* max number of user arguments for GLB */
}};
+/** Stagger each user argument value so we can detect mismatched
+ * indices.
+ *
+ * For example, say a plugin associates data with both sessions and
+ * transactions and that its session index is 2 and its transaction index is 4.
+ * In this case, we'll hand back to the plugin 2002 for its session index and
+ * 1004 for its transaction index. If it then accidentally uses its session
+ * index to reference its transaction index, it will pass back 2002 instead of
+ * 1004, which we will identify as belonging to the wrong user argument type
+ * because it is in the 2000 session block rather than the expected 1000
+ * transaction block.
+ *
+ * Note that these higher value 1000 block indices are only used when
+ * interfacing with the plugin. Internally the lower valued index is used.
+ * That is, for a transaction, expect internally a value of 3 instead of 1003.
+ */
+static constexpr std::array<size_t, TS_USER_ARGS_COUNT> USER_ARGS_OFFSETS = {{
+ 1000, /* max number of user arguments for TXN */
+ 2000, /* max number of user arguments for SSN */
+ 3000, /* max number of user arguments for VCONN */
+ 4000 /* max number of user arguments for GLB */
+}};
Review comment:
You could instead use:
```
static constexpr size_t USER_ARG_OFFSETS(TSUserArgType type)
{
return (static_cast<size_t>(type) + 1) * 1000;
}
```
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #8550: TSUserArg: add value type checking
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550#discussion_r765015644
##########
File path: include/tscore/PluginUserArgs.h
##########
@@ -35,6 +35,40 @@ static constexpr std::array<size_t, TS_USER_ARGS_COUNT> MAX_USER_ARGS = {{
128 /* max number of user arguments for GLB */
}};
+/** Stagger each user argument value so we can detect mismatched
+ * indices.
+ *
+ * For example, say a plugin associates data with both sessions and
+ * transactions and that its session index is 2 and its transaction index is 4.
+ * In this case, we'll hand back to the plugin 2002 for its session index and
+ * 1004 for its transaction index. If it then accidentally uses its session
+ * index to reference its transaction index, it will pass back 2002 instead of
+ * 1004, which we will identify as belonging to the wrong user argument type
+ * because it is in the 2000 session block rather than the expected 1000
+ * transaction block.
+ *
+ * Note that these higher value 1000 block indices are only used when
+ * interfacing with the plugin. Internally the lower valued index is used.
+ * That is, for a transaction, expect internally a value of 3 instead of 1003.
+ */
+static constexpr std::array<size_t, TS_USER_ARGS_COUNT> USER_ARGS_OFFSETS = {{
+ 1000, /* max number of user arguments for TXN */
+ 2000, /* max number of user arguments for SSN */
+ 3000, /* max number of user arguments for VCONN */
+ 4000 /* max number of user arguments for GLB */
+}};
+
+/** Verify that the user passed in an index whose value corresponds with the
+ * type. See the comment above the declaration of USER_ARGS_OFFSETS for the
+ * intention behind this.
+ */
+inline bool
+SanityCheckUserIndex(TSUserArgType type, int idx)
+{
Review comment:
I suggest adding:
```
if (static_cast<std::size_t>(type) >= MAX_USER_ARGS.size()) {
return false;
}
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #8550: TSUserArg: add value type checking
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550#issuecomment-989196240
> Nitpick: seems like static constexpr items should be private members of PluginUserArgsMixin. But that's perhaps beyond the scope of this PR.
I agree that it's good to keep stuff private if they can be, but these functions are both needed externally in the InkAPI.cc code. Here, for example:
https://github.com/apache/trafficserver/pull/8550/files#diff-e188d413739d07f1ad183ae5912ca435557809252154bdd5f2fcf625f95b9600R6331
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on pull request #8550: TSUserArg: add value type checking
Posted by GitBox <gi...@apache.org>.
masaori335 commented on pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550#issuecomment-993087481
[approve ci centos]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #8550: TSUserArg: add value type checking
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #8550:
URL: https://github.com/apache/trafficserver/pull/8550#issuecomment-1006149729
> I'd like to leave this out of 9.1.x if ok with you.
Fine with me. I'll remove the 9.1.x project.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org