Re: [PR] Make slcan brin the interface up/down with the appropiate commands [nuttx-apps]
xiaoxiang781216 merged PR #3059: URL: https://github.com/apache/nuttx-apps/pull/3059 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Make slcan brin the interface up/down with the appropiate commands [nuttx-apps]
xiaoxiang781216 commented on code in PR #3059: URL: https://github.com/apache/nuttx-apps/pull/3059#discussion_r2055618982 ## canutils/slcan/slcan.c: ## @@ -137,8 +137,7 @@ static int caninit(char *candev, int *s, struct sockaddr_can *addr, syslog(LOG_ERR, "Error opening CAN socket\n"); return -1; } - strncpy(ifr.ifr_name, candev, 4); - ifr.ifr_name[4] = '\0'; + strlcpy(ifr.ifr_name, candev, IFNAMSIZ); Review Comment: yes, strlcpy is used in many places, it's better to create a bunch of change to fix it. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Make slcan brin the interface up/down with the appropiate commands [nuttx-apps]
csanchezdll commented on code in PR #3059:
URL: https://github.com/apache/nuttx-apps/pull/3059#discussion_r2055591720
##
canutils/slcan/slcan.c:
##
@@ -137,8 +137,7 @@ static int caninit(char *candev, int *s, struct
sockaddr_can *addr,
syslog(LOG_ERR, "Error opening CAN socket\n");
return -1;
}
- strncpy(ifr.ifr_name, candev, 4);
- ifr.ifr_name[4] = '\0';
+ strlcpy(ifr.ifr_name, candev, IFNAMSIZ);
Review Comment:
Agreed `strscpy` is the way to go. However, it is IMHO out of scope if this
PR, which is required to keep slcan working after recent Nuttx changes. Options:
1) Leave the code totally untouched with the 4 limit length.
2) Leave `srtncpy` + explicit `\0` as it was originally (just change the max
length) [this was my original patch]
3) Use `strlcpy` [suggested by @xiaoxiang781216]
4) Port `strscpy` as part of this PR
5) Block this PR until `strscpy` is ported in another PR
I'd vote for 3. Otherwise 1 and 2 are fine. I think 4 and 5 are bad ideas.
##
canutils/slcan/slcan.c:
##
@@ -395,9 +407,22 @@ int main(int argc, char *argv[])
{
/* close interface */
- mode = 0;
- debug_print("Close interface\n");
- ok_return(fd);
+ struct ifreq ifr;
+
+ strlcpy(ifr.ifr_name, argv[1], IFNAMSIZ);
+
+ ifr.ifr_flags = IFF_DOWN;
Review Comment:
`IFF_DOWN` was removed in https://github.com/apache/nuttx/pull/13842 so
build checks failed. Fixing.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Make slcan brin the interface up/down with the appropiate commands [nuttx-apps]
xiaoxiang781216 commented on code in PR #3059: URL: https://github.com/apache/nuttx-apps/pull/3059#discussion_r2048429377 ## canutils/slcan/slcan.c: ## @@ -137,8 +137,7 @@ static int caninit(char *candev, int *s, struct sockaddr_can *addr, syslog(LOG_ERR, "Error opening CAN socket\n"); return -1; } - strncpy(ifr.ifr_name, candev, 4); - ifr.ifr_name[4] = '\0'; + strlcpy(ifr.ifr_name, candev, IFNAMSIZ); Review Comment: we can port safec library from: https://sourceforge.net/p/safeclib/code/ci/master/tree/ https://github.com/intel/safestringlib -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Make slcan brin the interface up/down with the appropiate commands [nuttx-apps]
acassis commented on code in PR #3059: URL: https://github.com/apache/nuttx-apps/pull/3059#discussion_r2046948817 ## canutils/slcan/slcan.c: ## @@ -137,8 +137,7 @@ static int caninit(char *candev, int *s, struct sockaddr_can *addr, syslog(LOG_ERR, "Error opening CAN socket\n"); return -1; } - strncpy(ifr.ifr_name, candev, 4); - ifr.ifr_name[4] = '\0'; + strlcpy(ifr.ifr_name, candev, IFNAMSIZ); Review Comment: @xiaoxiang781216 @csanchezdll after reading this article I think strscpy is the "way to go". As the author points: new code should use strscpy and strscpy_pad. Unfortunately NuttX doesn't support strscpy() yet. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Make slcan brin the interface up/down with the appropiate commands [nuttx-apps]
xiaoxiang781216 commented on code in PR #3059: URL: https://github.com/apache/nuttx-apps/pull/3059#discussion_r2046907885 ## canutils/slcan/slcan.c: ## @@ -137,8 +137,7 @@ static int caninit(char *candev, int *s, struct sockaddr_can *addr, syslog(LOG_ERR, "Error opening CAN socket\n"); return -1; } - strncpy(ifr.ifr_name, candev, 4); - ifr.ifr_name[4] = '\0'; + strlcpy(ifr.ifr_name, candev, IFNAMSIZ); Review Comment: strlcpy always add \0 to the destination buffer, so you don't need add '\0' again. DoS is another problem, if you want to address this problem, you need more safer api(strscpy): https://staticthinking.wordpress.com/2023/10/30/strcpy-strncpy-strlcpy-and-strscpy/ -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Make slcan brin the interface up/down with the appropiate commands [nuttx-apps]
acassis commented on code in PR #3059: URL: https://github.com/apache/nuttx-apps/pull/3059#discussion_r2046764328 ## canutils/slcan/slcan.c: ## @@ -137,8 +137,7 @@ static int caninit(char *candev, int *s, struct sockaddr_can *addr, syslog(LOG_ERR, "Error opening CAN socket\n"); return -1; } - strncpy(ifr.ifr_name, candev, 4); - ifr.ifr_name[4] = '\0'; + strlcpy(ifr.ifr_name, candev, IFNAMSIZ); Review Comment: @xiaoxiang781216 do you think these BUGS reported in the man pages aren't a concern: ``` BUGS All catenation functions share the same performance problem: Shlemiel the painter. As a mitigation, compilers are able to transform some calls to catenation functions into normal copy functions, since strlen(dst) is usually a byproduct of the previous copy. strlcpy(3) and strlcat(3) need to read the entire src string, even if the destination buffer is small. This makes them vulnerable to Denial of Service (DoS) attacks if an attacker can control the length of the src string. And if not, they're still unnec‐ essarily slow. ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Make slcan brin the interface up/down with the appropiate commands [nuttx-apps]
acassis commented on code in PR #3059: URL: https://github.com/apache/nuttx-apps/pull/3059#discussion_r2046756973 ## canutils/slcan/slcan.c: ## @@ -137,8 +137,7 @@ static int caninit(char *candev, int *s, struct sockaddr_can *addr, syslog(LOG_ERR, "Error opening CAN socket\n"); return -1; } - strncpy(ifr.ifr_name, candev, 4); - ifr.ifr_name[4] = '\0'; + strlcpy(ifr.ifr_name, candev, IFNAMSIZ); Review Comment: @csanchezdll my fault, I ran "man strlcpy" but only take a look at function description. In fact it seems a good solution. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Make slcan brin the interface up/down with the appropiate commands [nuttx-apps]
csanchezdll commented on code in PR #3059:
URL: https://github.com/apache/nuttx-apps/pull/3059#discussion_r2046705164
##
canutils/slcan/slcan.c:
##
@@ -137,8 +137,7 @@ static int caninit(char *candev, int *s, struct
sockaddr_can *addr,
syslog(LOG_ERR, "Error opening CAN socket\n");
return -1;
}
- strncpy(ifr.ifr_name, candev, 4);
- ifr.ifr_name[4] = '\0';
+ strlcpy(ifr.ifr_name, candev, IFNAMSIZ);
Review Comment:
This was here on my first version of the PR, with the original strncpy.
Changing to strlcpy and removing the explicit \0 addition was suggested by
@xiaoxiang781216 just above
https://github.com/apache/nuttx-apps/pull/3059#discussion_r2045139275.
I agree with him, strlcpy is safer, as it is guaranteed to add a \0 (strncpy
will only *copy* the original \0 if it fits in the buffer). Therefore, the
explicit \0 is no longer needed.
##
canutils/slcan/slcan.c:
##
@@ -316,9 +316,22 @@ int main(int argc, char *argv[])
{
/* open CAN interface */
- mode = 1;
- debug_print("Open interface\n");
- ok_return(fd);
+ struct ifreq ifr;
+
+ strlcpy(ifr.ifr_name, argv[1], IFNAMSIZ);
+
Review Comment:
Same here, strlcpy is garanteed to ad \0 even if the string source of the
copy is longer than the destination buffer.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Make slcan brin the interface up/down with the appropiate commands [nuttx-apps]
acassis commented on code in PR #3059:
URL: https://github.com/apache/nuttx-apps/pull/3059#discussion_r2046698164
##
canutils/slcan/slcan.c:
##
@@ -316,9 +316,22 @@ int main(int argc, char *argv[])
{
/* open CAN interface */
- mode = 1;
- debug_print("Open interface\n");
- ok_return(fd);
+ struct ifreq ifr;
+
+ strlcpy(ifr.ifr_name, argv[1], IFNAMSIZ);
+
Review Comment:
Please include the null termination here too. It will protect in cases where
users pass a longer name than IFNAMSIZ.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Make slcan brin the interface up/down with the appropiate commands [nuttx-apps]
acassis commented on code in PR #3059: URL: https://github.com/apache/nuttx-apps/pull/3059#discussion_r2046692911 ## canutils/slcan/slcan.c: ## @@ -137,8 +137,7 @@ static int caninit(char *candev, int *s, struct sockaddr_can *addr, syslog(LOG_ERR, "Error opening CAN socket\n"); return -1; } - strncpy(ifr.ifr_name, candev, 4); - ifr.ifr_name[4] = '\0'; + strlcpy(ifr.ifr_name, candev, IFNAMSIZ); Review Comment: @csanchezdll suggestion: include the ifr.irf_name[IFNAMSIZ] = '\0'; for safety reason, please also include it to line 365 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Make slcan brin the interface up/down with the appropiate commands [nuttx-apps]
csanchezdll commented on PR #3059:
URL: https://github.com/apache/nuttx-apps/pull/3059#issuecomment-2808719093
I fixed the format errors and used checkpatch from the nuttx repo.
I found the initialization of ifr = {0} that I had fails because format
rules want the braces on separate lines. Too much code for little meaning, so I
remove the initialization altogether and used = instead of |= when assigning
flags. The result is the same.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Make slcan brin the interface up/down with the appropiate commands [nuttx-apps]
csanchezdll commented on code in PR #3059: URL: https://github.com/apache/nuttx-apps/pull/3059#discussion_r2046318579 ## canutils/slcan/slcan.c: ## @@ -137,8 +137,8 @@ static int caninit(char *candev, int *s, struct sockaddr_can *addr, syslog(LOG_ERR, "Error opening CAN socket\n"); return -1; } - strncpy(ifr.ifr_name, candev, 4); - ifr.ifr_name[4] = '\0'; + strncpy(ifr.ifr_name, candev, IFNAMSIZ); + ifr.ifr_name[IFNAMSIZ] = '\0'; Review Comment: Makes sense. I was trying to make minimal changes, but this is better with strlcpy. Changed. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Make slcan brin the interface up/down with the appropiate commands [nuttx-apps]
xiaoxiang781216 commented on code in PR #3059: URL: https://github.com/apache/nuttx-apps/pull/3059#discussion_r2045139027 ## canutils/slcan/slcan.c: ## @@ -137,8 +137,8 @@ static int caninit(char *candev, int *s, struct sockaddr_can *addr, syslog(LOG_ERR, "Error opening CAN socket\n"); return -1; } - strncpy(ifr.ifr_name, candev, 4); - ifr.ifr_name[4] = '\0'; + strncpy(ifr.ifr_name, candev, IFNAMSIZ); Review Comment: strlcpy ## canutils/slcan/slcan.c: ## @@ -137,8 +137,8 @@ static int caninit(char *candev, int *s, struct sockaddr_can *addr, syslog(LOG_ERR, "Error opening CAN socket\n"); return -1; } - strncpy(ifr.ifr_name, candev, 4); - ifr.ifr_name[4] = '\0'; + strncpy(ifr.ifr_name, candev, IFNAMSIZ); + ifr.ifr_name[IFNAMSIZ] = '\0'; Review Comment: remove -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Make slcan brin the interface up/down with the appropiate commands [nuttx-apps]
nuttxpr commented on PR #3059: URL: https://github.com/apache/nuttx-apps/pull/3059#issuecomment-2806956844 [**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to meet the NuttX requirements, although some sections could be more explicit. **Strengths:** * **Clear Summary:** The summary explains the "why," "what," and "how" of the change, and links it to a prior NuttX PR. * **Impact on User:** Clearly explains the user impact (inability to change bitrate without the fix). * **Testing:** Provides a reasonable test setup and logs demonstrating the issue before and after the fix. The additional fix for interface names is also mentioned. **Areas for Improvement:** * **Summary:** While the related NuttX PR is linked, a related issue (if one exists) should also be linked. If not, state that there isn't a related issue. * **Impact:** While the user impact is clear, other impact sections are marked "NO" without explanation. Even if there's no impact, explicitly stating "NO - No change to the build process" is better for clarity. Be very explicit. * **Impact - Compatibility:** The summary mentions a backward compatibility issue with a prior NuttX change. This should be explicitly called out in the "Impact on compatibility" section. Is this change backward compatible with *previous* versions of slcan? * **Testing:** While the test setup is described, the host environment isn't fully specified (OS, compiler, etc.). While `can_6_14` is mentioned as a CAN interface on a custom board, more information on the board and the *target* architecture would be helpful. For instance: "Target: ARM, custom board (STM32F4xx based) with a CAN controller". Also specify the NuttX version being tested against. **Example of Improved Impact Section:** ``` ## Impact * Is new feature added? NO * Is existing feature changed? YES, the bitrate setting logic is modified to accommodate a recent change in the NuttX CAN driver. * Impact on user (will user need to adapt to change)? NO. Users who were previously using the bitrate setting feature of slcan will find it working correctly again. * Impact on build (will build process change)? NO - No changes to the build process. * Impact on hardware (will arch(s) / board(s) / driver(s) change)? NO - No changes to hardware or drivers. This change affects the slcan application's interaction with the CAN driver. * Impact on documentation (is update required / provided)? NO - No documentation updates required. * Impact on security (any sort of implications)? NO - No security implications. * Impact on compatibility (backward/forward/interoperability)? YES - This change restores compatibility with a recent change in NuttX (https://github.com/apache/nuttx/pull/16199) that broke the bitrate setting functionality in slcan. This change is backward compatible with previous versions of slcan that were working correctly before the NuttX change. * Anything else to consider? The maximum length of the CAN interface name supported by slcan has been increased (addressed in a separate commit). ``` By being more thorough and explicit in the Impact and Testing sections, the PR will be easier to review and merge. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
