You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/07/12 20:02:49 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request #4124: add #undef for some libc function
xiaoxiang781216 opened a new pull request #4124:
URL: https://github.com/apache/incubator-nuttx/pull/4124
## Summary
since it's useful to redirect these functions to others
sometime(e.g. validate the memory before write).
## Impact
No, same as before
## Testing
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4124: add #undef for some libc function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4124:
URL: https://github.com/apache/incubator-nuttx/pull/4124#issuecomment-878327795
> Could you describe your use case with a bit more depth?
> My first impression is that this seems an ugly solution for performing code instrumentation.
Here is a case: we implement a special heap manager which contain many information about the allocation:
1. The malloc call stack, thread id...
2. The free call stack, thrad id...
3. The redzone before and after the memory to catch the overwrite
so, the manager can report the useful info when the memory corruption happen, but it would be better to catch the corruption more proactive than passive by hooking the memory manipulation function as much as possible like this:
1. Define strcpy to strcpy_chk
2. Check the source and destition pointer/size is valid in strcpy_chk
3. Show the detailed info in case of check failure
4. Call the original strcpy if pass
The above technique is used in many libc implementation.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #4124: add #undef for some libc function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #4124:
URL: https://github.com/apache/incubator-nuttx/pull/4124#issuecomment-879146380
> @xiaoxiang781216 Have you considered renaming the standard functions as `nx_<function>` (e.g. `nx_malloc`) and defining `<function>` as a weak symbol when `CONFIG_MM_DEBUG` is enabled? Then the weak `malloc` by default only calls `nx_malloc`
The hook for malloc/free is already done by: https://github.com/apache/incubator-nuttx/pull/3086.
This PR try to hook other memory related functions(e.g. strcpy, strlen, memcpy...).
> and any available checkers could simply provide a `malloc` implementation. Something like this:
>
> ```c
> #ifdef CONFIG_MM_DEBUG
> weak_function FAR void *malloc(size_t size)
> {
> return nx_malloc(size);
> }
>
> FAR void *nx_malloc(size_t size)
> #else
> FAR void *malloc(size_t size)
> {
> /* Default implementation */
> }
> #endif
> ```
>
> This approach would have the advantage of being explicit of what this is intending to achieve, but at the cost of performing this mass renaming of target functions.
> That was I was referring to when I said that we wouldn't need to rely on the preprocessor for aliasing function names, preventing the mass `#undef`.
The problem is that many hook functions want to get more context info from call site(e.g. file name, line number), only macro can achieve this trick in a portable way just like how assert implementation.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #4124: add #undef for some libc function
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #4124:
URL: https://github.com/apache/incubator-nuttx/pull/4124#issuecomment-879080308
@xiaoxiang781216 - your argument is sound. The implementation is ugly. In the past I know that @patacongo rejected certain kinds of debug code in the codebase. I think the value far out ways the the ugly-ness. I would ask that you add a readme in the docs that full explains how to use this AND on each of the #undef add a /* See <path to readme> */
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4124: add #undef for some libc function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4124:
URL: https://github.com/apache/incubator-nuttx/pull/4124#issuecomment-880036113
> @xiaoxiang781216 - your argument is sound. The implementation is ugly. In the past I know that @patacongo rejected certain kinds of debug code in the codebase. I think the value far out ways the the ugly-ness. I would ask that you add a readme in the docs that full explains how to use this AND on each of the #undef add a /* See < path to readme > */
Done, @davids5 and @gustavonihei please review again.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #4124: add #undef for some libc function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #4124:
URL: https://github.com/apache/incubator-nuttx/pull/4124#issuecomment-878745290
> > 1. Define strcpy to strcpy_chk
> > 2. Check the source and destition pointer/size is valid in strcpy_chk
> > 3. Show the detailed info in case of check failure
> > 4. Call the original strcpy if pass
> >
> > The above technique is used in many libc implementation.
>
> Indeed, I've seen this technique being employed in several places, but the way it is proposed in this PR seems like a workaround, spreading `#undef`s for cleaning up the preprocessor "namespace" which was polluted from an out-of-tree modification. This does not seem right.
In tree version also need it too, see the comment below.
> For this scenario, I agree with @davids5's suggestion, the linking-time approach using the `wrap` command seems more suitable.
>
--wrap is a gcc specific feature, even clang doesn't support yet:
https://stackoverflow.com/questions/31476632/what-is-the-equivalent-of-gnus-wrap-linker-flag-in-os-x-linker
And then violate https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md:
## All Users Matter
- Supported toolchains: GCC, Clang, SDCC, ZiLOG ZDS-II (c89), IAR.
- We must resist the pull to make NuttX into a Linux-only, GCC-only, and
ARM-only solution.
> If the idea is to prepare for a future in-tree OS feature (e.g. an address sanitizer), then I'd go with the compile-time approach, but since NuttX builds its own version of allocators, we don't need to rely on the preprocessor for performing the function interposition.
Even with the builtin allocator, this approach may more cleaner than the initial thinking. Otherwise, we have to modify all related functions like this in many standard header files:
```
#ifdef CONFIG_MM_DEBUG
#define strcpy(d, s, l) strcpy_chk(__FILE__, __LINE__, d, s, l)
#endif
```
But with this patch, all these replacement could gather into one header file(e.g. xxx_debug.h). More important, since NuttX support the custom heap manager now, this approach is crucial to make the hook come from the different heap manager coexist with each other peacefully(e.g. xxx_debug.h and yyy_debug.h).
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #4124: add #undef for some libc function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #4124:
URL: https://github.com/apache/incubator-nuttx/pull/4124#issuecomment-878745290
> Indeed, I've seen this technique being employed in several places, but the way it is proposed in this PR seems like a workaround, spreading `#undef`s for cleaning up the preprocessor "namespace" which was polluted from an out-of-tree modification. This does not seem right.
In tree version also need it too, see the comment below.
> For this scenario, I agree with @davids5's suggestion, the linking-time approach using the `wrap` command seems more suitable.
>
--wrap is a gcc specific feature, even clang doesn't support yet:
https://stackoverflow.com/questions/31476632/what-is-the-equivalent-of-gnus-wrap-linker-flag-in-os-x-linker
And then violate https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md:
```
## All Users Matter
- Supported toolchains: GCC, Clang, SDCC, ZiLOG ZDS-II (c89), IAR.
- We must resist the pull to make NuttX into a Linux-only, GCC-only, and
ARM-only solution.
```
> If the idea is to prepare for a future in-tree OS feature (e.g. an address sanitizer), then I'd go with the compile-time approach, but since NuttX builds its own version of allocators, we don't need to rely on the preprocessor for performing the function interposition.
Even with the builtin allocator, this approach may more cleaner than the initial thinking. Otherwise, we have to modify all related functions like this in many standard header files(we still need to `#undef strcpy` in lib_strcpy.c, do you have a better method?):
```
#ifdef CONFIG_MM_DEBUG
#define strcpy(d, s, l) strcpy_chk(__FILE__, __LINE__, d, s, l)
#endif
```
But with this patch, all these replacement could gather into one header file(e.g. xxx_debug.h). More important, since NuttX support the custom heap manager now, this approach is crucial to make the hook come from the different heap manager coexist with each other peacefully. e.g. xxx_debug.h/yyy_debug.h v.s. this:
```
#ifdef CONFIG_MM_DEBUG_XXX
#define strcpy(d, s, l) strcpy_chk_xxx(__FILE__, __LINE__, d, s, l)
#elif define(CONFIG_MM_DEBUG_YYY
#define strcpy(d, s, l) strcpy_chk_yyy(d, s, l)
...
#endif
```
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #4124: add #undef for some libc function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #4124:
URL: https://github.com/apache/incubator-nuttx/pull/4124#issuecomment-878327795
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #4124: add #undef for some libc function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #4124:
URL: https://github.com/apache/incubator-nuttx/pull/4124#issuecomment-878745290
> Indeed, I've seen this technique being employed in several places, but the way it is proposed in this PR seems like a workaround, spreading `#undef`s for cleaning up the preprocessor "namespace" which was polluted from an out-of-tree modification. This does not seem right.
In tree version also need it too, see the comment below.
> For this scenario, I agree with @davids5's suggestion, the linking-time approach using the `wrap` command seems more suitable.
>
--wrap is a gcc specific feature, even clang doesn't support yet:
https://stackoverflow.com/questions/31476632/what-is-the-equivalent-of-gnus-wrap-linker-flag-in-os-x-linker
And then violate https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md:
```
## All Users Matter
- Supported toolchains: GCC, Clang, SDCC, ZiLOG ZDS-II (c89), IAR.
- We must resist the pull to make NuttX into a Linux-only, GCC-only, and
ARM-only solution.
```
> If the idea is to prepare for a future in-tree OS feature (e.g. an address sanitizer), then I'd go with the compile-time approach, but since NuttX builds its own version of allocators, we don't need to rely on the preprocessor for performing the function interposition.
Even with the builtin allocator, this approach may more cleaner than the initial thinking. Otherwise, we have to modify all related functions like this in many standard header files:
```
#ifdef CONFIG_MM_DEBUG
#define strcpy(d, s, l) strcpy_chk(__FILE__, __LINE__, d, s, l)
#endif
```
But with this patch, all these replacement could gather into one header file(e.g. xxx_debug.h). More important, since NuttX support the custom heap manager now, this approach is crucial to make the hook come from the different heap manager coexist with each other peacefully. e.g. xxx_debug.h/yyy_debug.h v.s. this:
```
#ifdef CONFIG_MM_DEBUG_XXX
#define strcpy(d, s, l) strcpy_chk_xxx(__FILE__, __LINE__, d, s, l)
#elif define(CONFIG_MM_DEBUG_YYY
#define strcpy(d, s, l) strcpy_chk_yyy(d, s, l)
#endif
```
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 edited a comment on pull request #4124: add #undef for some libc function
Posted by GitBox <gi...@apache.org>.
davids5 edited a comment on pull request #4124:
URL: https://github.com/apache/incubator-nuttx/pull/4124#issuecomment-879080308
@xiaoxiang781216 - your argument is sound. The implementation is ugly. In the past I know that @patacongo rejected certain kinds of debug code in the codebase. I think the value far out ways the the ugly-ness. I would ask that you add a readme in the docs that full explains how to use this AND on each of the #undef add a /* See < path to readme > */
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on pull request #4124: add #undef for some libc function
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #4124:
URL: https://github.com/apache/incubator-nuttx/pull/4124#issuecomment-878616684
> 1. Define strcpy to strcpy_chk
> 2. Check the source and destition pointer/size is valid in strcpy_chk
> 3. Show the detailed info in case of check failure
> 4. Call the original strcpy if pass
>
> The above technique is used in many libc implementation.
Indeed, I've seen this technique being employed in several places, but the way it is proposed in this PR seems like a workaround, spreading `#undef`s for cleaning up the preprocessor "namespace" which was polluted from an out-of-tree modification. This does not seem right.
For this scenario, I agree with @davids5's suggestion, the linking-time approach using the `wrap` command seems more suitable.
If the idea is to prepare for a future in-tree OS feature (e.g. an address sanitizer), then I'd go with the compile-time approach, but since NuttX builds its own version of allocators, we don't need to rely on the preprocessor for performing the function interposition.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #4124: add #undef for some libc function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #4124:
URL: https://github.com/apache/incubator-nuttx/pull/4124#issuecomment-878745290
> Indeed, I've seen this technique being employed in several places, but the way it is proposed in this PR seems like a workaround, spreading `#undef`s for cleaning up the preprocessor "namespace" which was polluted from an out-of-tree modification. This does not seem right.
In tree version also need it too, see the comment below.
> For this scenario, I agree with @davids5's suggestion, the linking-time approach using the `wrap` command seems more suitable.
>
--wrap is a gcc specific feature, even clang doesn't support yet:
https://stackoverflow.com/questions/31476632/what-is-the-equivalent-of-gnus-wrap-linker-flag-in-os-x-linker
And then violate https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md:
```
## All Users Matter
- Supported toolchains: GCC, Clang, SDCC, ZiLOG ZDS-II (c89), IAR.
- We must resist the pull to make NuttX into a Linux-only, GCC-only, and
ARM-only solution.
```
> If the idea is to prepare for a future in-tree OS feature (e.g. an address sanitizer), then I'd go with the compile-time approach, but since NuttX builds its own version of allocators, we don't need to rely on the preprocessor for performing the function interposition.
Even with the builtin allocator, this approach may more cleaner than the initial thinking. Otherwise, we have to modify all related functions like this in many standard header files:
```
#ifdef CONFIG_MM_DEBUG
#define strcpy(d, s, l) strcpy_chk(__FILE__, __LINE__, d, s, l)
#endif
```
But with this patch, all these replacement could gather into one header file(e.g. xxx_debug.h). More important, since NuttX support the custom heap manager now, this approach is crucial to make the hook come from the different heap manager coexist with each other peacefully(e.g. xxx_debug.h and yyy_debug.h).
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei edited a comment on pull request #4124: add #undef for some libc function
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #4124:
URL: https://github.com/apache/incubator-nuttx/pull/4124#issuecomment-879100326
> --wrap is a gcc specific feature, even clang doesn't support yet:
> https://stackoverflow.com/questions/31476632/what-is-the-equivalent-of-gnus-wrap-linker-flag-in-os-x-linker
> And then violate https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md:
Good point. I was unaware that LLVM's Linker did not implement it.
@xiaoxiang781216 Have you considered renaming the standard functions as `nx_<function>` (e.g. `nx_malloc`) and defining `<function>` as a weak symbol when `CONFIG_MM_DEBUG` is enabled? Then the weak `malloc` by default only calls `nx_malloc` and any available checkers could simply provide a `malloc` implementation. Something like:
```c
#ifdef CONFIG_MM_DEBUG
weak_function FAR void *malloc(size_t size)
{
return nx_malloc(size);
}
FAR void *nx_malloc(size_t size)
#else
FAR void *malloc(size_t size)
{
/* Default implementation */
}
#endif
```
This approach would have the advantage of being explicit of what this is intending to achieve, but at the cost of performing this mass renaming of target functions.
That was I was referring to when I said that we wouldn't need to rely on the preprocessor.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #4124: add #undef for some libc function
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #4124:
URL: https://github.com/apache/incubator-nuttx/pull/4124#issuecomment-878373592
@xiaoxiang781216 - why not use the linker's wrap and not change the source code source for this sort of testing?
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #4124: add #undef for some libc function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #4124:
URL: https://github.com/apache/incubator-nuttx/pull/4124#issuecomment-878745290
> Indeed, I've seen this technique being employed in several places, but the way it is proposed in this PR seems like a workaround, spreading `#undef`s for cleaning up the preprocessor "namespace" which was polluted from an out-of-tree modification. This does not seem right.
In tree version also need it too, see the comment below.
> For this scenario, I agree with @davids5's suggestion, the linking-time approach using the `wrap` command seems more suitable.
>
--wrap is a gcc specific feature, even clang doesn't support yet:
https://stackoverflow.com/questions/31476632/what-is-the-equivalent-of-gnus-wrap-linker-flag-in-os-x-linker
And then violate https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md:
```
## All Users Matter
- Supported toolchains: GCC, Clang, SDCC, ZiLOG ZDS-II (c89), IAR.
- We must resist the pull to make NuttX into a Linux-only, GCC-only, and
ARM-only solution.
```
> If the idea is to prepare for a future in-tree OS feature (e.g. an address sanitizer), then I'd go with the compile-time approach, but since NuttX builds its own version of allocators, we don't need to rely on the preprocessor for performing the function interposition.
Even with the builtin allocator, this approach may more cleaner than the initial thinking. Otherwise, we have to modify all related functions like this in many standard header files:
```
#ifdef CONFIG_MM_DEBUG
#define strcpy(d, s, l) strcpy_chk(__FILE__, __LINE__, d, s, l)
#endif
```
But with this patch, all these replacement could gather into one header file(e.g. xxx_debug.h). More important, since NuttX support the custom heap manager now, this approach is crucial to make the hook come from the different heap manager coexist with each other peacefully. e.g. xxx_debug.h/yyy_debug.h v.s. this:
```
#ifdef CONFIG_MM_DEBUG_XXX
#define strcpy(d, s, l) strcpy_chk_xxx(__FILE__, __LINE__, d, s, l)
#elif define(CONFIG_MM_DEBUG_YYY
#define strcpy(d, s, l) strcpy_chk_yyy(d, s, l)
...
#endif
```
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4124: add #undef for some libc function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4124:
URL: https://github.com/apache/incubator-nuttx/pull/4124#issuecomment-878745290
> > 1. Define strcpy to strcpy_chk
> > 2. Check the source and destition pointer/size is valid in strcpy_chk
> > 3. Show the detailed info in case of check failure
> > 4. Call the original strcpy if pass
> >
> > The above technique is used in many libc implementation.
>
> Indeed, I've seen this technique being employed in several places, but the way it is proposed in this PR seems like a workaround, spreading `#undef`s for cleaning up the preprocessor "namespace" which was polluted from an out-of-tree modification. This does not seem right.
> For this scenario, I agree with @davids5's suggestion, the linking-time approach using the `wrap` command seems more suitable.
>
--wrap is a gcc specific feature, even clang doesn't support yet:
https://stackoverflow.com/questions/31476632/what-is-the-equivalent-of-gnus-wrap-linker-flag-in-os-x-linker
And then violate https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md:
## All Users Matter
- Supported toolchains: GCC, Clang, SDCC, ZiLOG ZDS-II (c89), IAR.
- We must resist the pull to make NuttX into a Linux-only, GCC-only, and
ARM-only solution.
> If the idea is to prepare for a future in-tree OS feature (e.g. an address sanitizer), then I'd go with the compile-time approach, but since NuttX builds its own version of allocators, we don't need to rely on the preprocessor for performing the function interposition.
Even with the builtin allocator, this approach may more cleaner than the initial thinking. Otherwise, we have to modify all related functions like this in many standard header files:
```
#ifdef CONFIG_MM_DEBUG
#define strcpy(d, s, l) strcpy_chk(__FILE__, __LINE__, d, s, l)
#endif
```
But with this patch, all these replacement could gather into one header file(e.g. xxx_debug.h). More important, since NuttX support the custom heap manager now, this approach is crucial to make the hook come from the different heap manager coexist with each other peacefully(e.g. xxx_debug.h and yyy_debug.h).
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 edited a comment on pull request #4124: add #undef for some libc function
Posted by GitBox <gi...@apache.org>.
davids5 edited a comment on pull request #4124:
URL: https://github.com/apache/incubator-nuttx/pull/4124#issuecomment-879080308
@xiaoxiang781216 - your argument is sound. The implementation is ugly. In the past I know that @patacongo rejected certain kinds of debug code in the codebase. I think the value far out ways the the ugly-ness. I would ask that you add a readme in the docs that full explains how to use this AND on each of the #undef add a /* See /<path to readme/> */
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4124: add #undef for some libc function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4124:
URL: https://github.com/apache/incubator-nuttx/pull/4124#issuecomment-879146380
The hook for malloc/free is already done by: https://github.com/apache/incubator-nuttx/pull/3086.
This PR try to hook other memory related functions(e.g. strcpy, strlen, memcpy...).
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei merged pull request #4124: add #undef for some libc function
Posted by GitBox <gi...@apache.org>.
gustavonihei merged pull request #4124:
URL: https://github.com/apache/incubator-nuttx/pull/4124
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei edited a comment on pull request #4124: add #undef for some libc function
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #4124:
URL: https://github.com/apache/incubator-nuttx/pull/4124#issuecomment-879100326
> --wrap is a gcc specific feature, even clang doesn't support yet:
> https://stackoverflow.com/questions/31476632/what-is-the-equivalent-of-gnus-wrap-linker-flag-in-os-x-linker
> And then violate https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md:
Good point. I was unaware that LLVM's Linker did not implement it.
@xiaoxiang781216 Have you considered renaming the standard functions as `nx_<function>` (e.g. `nx_malloc`) and defining `<function>` as a weak symbol when `CONFIG_MM_DEBUG` is enabled? Then the weak `malloc` by default only calls `nx_malloc` and any available checkers could simply provide a `malloc` implementation. Something like this:
```c
#ifdef CONFIG_MM_DEBUG
weak_function FAR void *malloc(size_t size)
{
return nx_malloc(size);
}
FAR void *nx_malloc(size_t size)
#else
FAR void *malloc(size_t size)
{
/* Default implementation */
}
#endif
```
This approach would have the advantage of being explicit of what this is intending to achieve, but at the cost of performing this mass renaming of target functions.
That was I was referring to when I said that we wouldn't need to rely on the preprocessor for aliasing function names, preventing the mass `#undef`.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on pull request #4124: add #undef for some libc function
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #4124:
URL: https://github.com/apache/incubator-nuttx/pull/4124#issuecomment-878292947
Could you describe your use case with a bit more depth?
My first impression is that this seems an ugly solution for performing code instrumentation.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on pull request #4124: add #undef for some libc function
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #4124:
URL: https://github.com/apache/incubator-nuttx/pull/4124#issuecomment-879100326
> --wrap is a gcc specific feature, even clang doesn't support yet:
> https://stackoverflow.com/questions/31476632/what-is-the-equivalent-of-gnus-wrap-linker-flag-in-os-x-linker
> And then violate https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md:
Good point. I was unaware that LLVM's Linker did not implement.
@xiaoxiang781216 Have you considered renaming the standard functions as `nx_<function>` (e.g. `nx_malloc`) and defining `<function>` as a weak symbol when `CONFIG_MM_DEBUG` is enabled? Then the weak `malloc` by default only calls `nx_malloc` and any available checkers could simply provide a `malloc` implementation. Something like:
```c
#ifdef CONFIG_MM_DEBUG
weak_function FAR void *malloc(size_t size)
{
return nx_malloc(size);
}
FAR void *nx_malloc(size_t size)
#else
FAR void *malloc(size_t size)
{
/* Default implementation */
}
#endif
```
This approach would have the advantage of being explicit of what this is intending to achieve, but at the cost of performing this mass renaming of target functions.
That was I was referring to when I said that we wouldn't need to rely on the preprocessor.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei edited a comment on pull request #4124: add #undef for some libc function
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #4124:
URL: https://github.com/apache/incubator-nuttx/pull/4124#issuecomment-879100326
> --wrap is a gcc specific feature, even clang doesn't support yet:
> https://stackoverflow.com/questions/31476632/what-is-the-equivalent-of-gnus-wrap-linker-flag-in-os-x-linker
> And then violate https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md:
Good point. I was unaware that LLVM's Linker did not implement it.
@xiaoxiang781216 Have you considered renaming the standard functions as `nx_<function>` (e.g. `nx_malloc`) and defining `<function>` as a weak symbol when `CONFIG_MM_DEBUG` is enabled? Then the weak `malloc` by default only calls `nx_malloc` and any available checkers could simply provide a `malloc` implementation. Something like this:
```c
#ifdef CONFIG_MM_DEBUG
weak_function FAR void *malloc(size_t size)
{
return nx_malloc(size);
}
FAR void *nx_malloc(size_t size)
#else
FAR void *malloc(size_t size)
{
/* Default implementation */
}
#endif
```
This approach would have the advantage of being explicit of what this is intending to achieve, but at the cost of performing this mass renaming of target functions.
That was I was referring to when I said that we wouldn't need to rely on the preprocessor.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #4124: add #undef for some libc function
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #4124:
URL: https://github.com/apache/incubator-nuttx/pull/4124#issuecomment-878745290
> > 1. Define strcpy to strcpy_chk
> > 2. Check the source and destition pointer/size is valid in strcpy_chk
> > 3. Show the detailed info in case of check failure
> > 4. Call the original strcpy if pass
> >
> > The above technique is used in many libc implementation.
>
> Indeed, I've seen this technique being employed in several places, but the way it is proposed in this PR seems like a workaround, spreading `#undef`s for cleaning up the preprocessor "namespace" which was polluted from an out-of-tree modification. This does not seem right.
In tree version also need it too, see the comment below.
> For this scenario, I agree with @davids5's suggestion, the linking-time approach using the `wrap` command seems more suitable.
>
--wrap is a gcc specific feature, even clang doesn't support yet:
https://stackoverflow.com/questions/31476632/what-is-the-equivalent-of-gnus-wrap-linker-flag-in-os-x-linker
And then violate https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md:
```
## All Users Matter
- Supported toolchains: GCC, Clang, SDCC, ZiLOG ZDS-II (c89), IAR.
- We must resist the pull to make NuttX into a Linux-only, GCC-only, and
ARM-only solution.
```
> If the idea is to prepare for a future in-tree OS feature (e.g. an address sanitizer), then I'd go with the compile-time approach, but since NuttX builds its own version of allocators, we don't need to rely on the preprocessor for performing the function interposition.
Even with the builtin allocator, this approach may more cleaner than the initial thinking. Otherwise, we have to modify all related functions like this in many standard header files:
```
#ifdef CONFIG_MM_DEBUG
#define strcpy(d, s, l) strcpy_chk(__FILE__, __LINE__, d, s, l)
#endif
```
But with this patch, all these replacement could gather into one header file(e.g. xxx_debug.h). More important, since NuttX support the custom heap manager now, this approach is crucial to make the hook come from the different heap manager coexist with each other peacefully(e.g. xxx_debug.h and yyy_debug.h).
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei edited a comment on pull request #4124: add #undef for some libc function
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #4124:
URL: https://github.com/apache/incubator-nuttx/pull/4124#issuecomment-879100326
> --wrap is a gcc specific feature, even clang doesn't support yet:
> https://stackoverflow.com/questions/31476632/what-is-the-equivalent-of-gnus-wrap-linker-flag-in-os-x-linker
> And then violate https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md:
Good point. I was unaware that LLVM's Linker did not implement it.
@xiaoxiang781216 Have you considered renaming the standard functions as `nx_<function>` (e.g. `nx_malloc`) and defining `<function>` as a weak symbol when `CONFIG_MM_DEBUG` is enabled? Then the weak `malloc` by default only calls `nx_malloc` and any available checkers could simply provide a `malloc` implementation. Something like this:
```c
#ifdef CONFIG_MM_DEBUG
weak_function FAR void *malloc(size_t size)
{
return nx_malloc(size);
}
FAR void *nx_malloc(size_t size)
#else
FAR void *malloc(size_t size)
{
/* Default implementation */
}
#endif
```
This approach would have the advantage of being explicit of what this is intending to achieve, but at the cost of performing this mass renaming of target functions.
That was I was referring to when I said that we wouldn't need to rely on the preprocessor for aliasing function names.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org