Make sure argv indeces exist before comparing them in test_lfs.c and test_time.c #91
Conversation
There was a problem hiding this comment.
Left a suggestion to consider.
Also- this is 100% not your fault as we need to still set up a lot of the guidance and procedures for what we'd eventually like to see in PR's.
But just to get started- PR's are much more friendly to the reviewer if you:
- Describe (just a short summary with link to applicable issues) what the PR is for.
- If there's anything un-intuitive happening in the code, describe the design and your thought process behind the design (make sure your code is sound, in general).
- Prescribe whether or not your code is polished. It's ok if it's not, just list out things that ought to be done in future refactors and/or make those issue for tracking.
| time_t unix_time = rtcGetTimeUnix(&msec); | ||
| char *timestr = ctime(&unix_time); | ||
| chprintf(chp, "UNIX Time: %d\r\n" | ||
| "Date: %s\r\n", | ||
| unix_time, timestr); | ||
| } else if (!strcmp(argv[1], "set") && argc > 2) { | ||
| } else if (argv[1] != NULL && !strcmp(argv[1], "set") && argc > 2) { |
There was a problem hiding this comment.
In this (and similar) situations this actually creates a new subtle bug. You're checking if the value at the location is zero'ed out- which sometimes will be true- sometimes wont. This also assumes that this is in-bounds.
Me thinks it would be better to check against argc and ensure it is greater than or equal to the current spot you are trying to deference.
For example: argv[0] is technically always a valid pointer, but it's also a de-reference operation that's not guaranteed to work if the array was never allocated, or not allocated with the size you assume. This code would break if we simply passed no args to this tool.
There was a problem hiding this comment.
I've updated the description for now, will make it more detailed ASAP. Will also make this conditional/similar more sturdy, likely today or tomorrow.
Thanks!
ghost
marked this pull request as draft
|
With #99 are we dropping support for F4 apps, so this is no longer needed |
ghost
closed this
This should resolve issue #89