Add checks in FreeRTOS_Socket.c #104
Conversation
|
|
||
| xByteCount = ( BaseType_t ) prvTCPSendCheck( pxSocket, uxDataLength ); | ||
|
|
||
| if (pvBuffer != NULL) |
There was a problem hiding this comment.
-if (pvBuffer != NULL)
+if( pvBuffer != NULL )| ( pvBuffer == NULL ) ) | ||
| { | ||
| xByteCount = -pdFREERTOS_ERRNO_EINVAL; | ||
| xByteCount = FREERTOS_EINVAL; |
There was a problem hiding this comment.
There is one situation in which it is valid to set pvBuffer to NULL: after the data have been read with the flag FREERTOS_ZERO_COPY.
Checking prvValidSocket() here should be enough.
yuhui-zheng
left a comment
yuhui-zheng
left a comment
There was a problem hiding this comment.
Out of scope for this PR, but may want to clean up. I found two FreeRTOS_errno_TCP.h files under /FreeRTOS-Plus directory.
➜ FreeRTOS-Plus git:(pr_104) ✗ find . -name FreeRTOS_errno_TCP.h -print
./Test/Unit-Tests/Config_files/FreeRTOS_errno_TCP.h
./Source/FreeRTOS-Plus-TCP/include/FreeRTOS_errno_TCP.h
|
Thanks @yuhui-zheng for pointing that out. I'll remove it in a different PR. The Unit tests folder has many files which were copied there from FreeRTOS-Plus-TCP. I will remove them. |
|
In /* Check if the socket is valid, has type TCP and if it is bound to a
port. */
if( prvValidSocket( pxSocket, FREERTOS_IPPROTO_TCP, pdTRUE ) == pdFALSE )
{
xByteCount = -pdFREERTOS_ERRNO_EINVAL;
}
+ else if( ( ( ( uint32_t ) xFlags & ( uint32_t ) FREERTOS_ZERO_COPY ) != 0U ) &&
+ ( pvBuffer == NULL ) )
+ {
+ /* In zero-copy mode, pvBuffer is a pointer to a pointer ( not NULL ). */
+ xByteCount = -pdFREERTOS_ERRNO_EINVAL;
+ }
else
{Beside that I think the PR is all good. |
AniruddhaKanhere
dismissed stale reviews from gkwicker and yuhui-zheng
via
7c508e0
yuhui-zheng
left a comment
yuhui-zheng
left a comment
There was a problem hiding this comment.
This would be a blind approval from me. For code paths taken on each scenario, I'll lean on the advices from experts in these thread.
gkwicker
left a comment
gkwicker
left a comment
There was a problem hiding this comment.
One question, see comments
| ( void ) xAddressLength; | ||
|
|
||
| xResult = prvTCPConnectStart( pxSocket, pxAddress ); | ||
| if( pxAddress != NULL ) |
There was a problem hiding this comment.
Why are we validating pxAddress but not pxSocket or xAddressLength?
There was a problem hiding this comment.
pxSocket is validated by prvTCPConnectStart which checks fo the socket being NULL or FREERTOS_INVALID_SOCKET.
xAddressLength is not used at all in this implementation. See a couple of lines above this comment: ( void ) xAddressLength; is used to ignore the parameter xAddressLength.
There was a problem hiding this comment.
Then if prvTCPConnectStart() validates one parameter pxSocket why doesn't it validate both of them? It is weird validating one parameter in each routine, why not just do the validations in one place?
There was a problem hiding this comment.
I agree with you on this front. prvTCPConnectStart() is used only in this particular function so it can be easily modified without any repercussions. I'll update that!
Thanks :)
* Fixed Imports for Infineon XMC1100 Board (#88) Co-authored-by: RichardBarry <[email protected]> * FreeRTOS+TCP : add memory statistics and dump packets, v3 (#83) * FreeRTOS+TCP : add memory statistics and dump packets, v3 * Two changes as requested by Aniruddha Co-authored-by: Hein Tibosch <[email protected]> Co-authored-by: Aniruddha Kanhere <[email protected]> * Folder structure change + Fix broken Projects (#103) * Update folder structure * Correct project files * Move test folder * Some changes after Yuki's comments * Add checks in FreeRTOS_Socket.c (#104) * Add fail-safes to FreeRTOS_Socket.c * Use all 'pd' errors * Correction after Hein's comments * Correction after Hein's comments v2 * Changes after Hein's comments * Update after Gary's comments * Add VeriFast kernel queue proofs (#117) * Remove unnecessary semicolon from the linker file (#121) This was creating problem with the onboard LPCLink debug probe. Signed-off-by: Gaurav Aggarwal <[email protected]> * Add Full TCP test suite - not using secure sockets (#131) * Add Full-TCP suite * delete unnecessary files * Change after Joshua's comments * Add changes from 2225-2227 amazon-FreeRTOS (#134) * FreeRTOS+TCP Adding the combined driver for SAM4E and SAME70 v2 (#78) * Adding a combined +TCP driver for SAM4E and SAME70 * Changes after review from Aniruddha Co-authored-by: Hein Tibosch <[email protected]> Co-authored-by: Aniruddha Kanhere <[email protected]> * Prove buffer lemmas (#124) * Prove buffer lemmas * Update queue proofs to latest kernel source All changes were syntactic due to uncrustify code-formatting * Strengthen prvCopyDataToQueue proof * Add extract script for diff comparison Co-authored-by: Yuhui Zheng <[email protected]> * Sync with +TCP amazon-FreeRTOS (#158) * DNS.c commit * IP.c commit * Add various source & header files * Add Uncrustify file used for Kernel. (#163) * Add Atmel Studio projects for ATMega4809 and AVR128DA48 (#159) * Added explicit cast to allow roll over and avoid integer promotion during cycles counters comparison in recmutex.c. * Fixed type mismatch between declaration and definition of function xAreSemaphoreTasksStillRunning( void ). * Added Atmel Studio demo projects for ATMega4809 and AVR128DA48. * Per https://www.freertos.org/upgrading-to-FreeRTOS-V8.html, I'm updating portBASE_TYPE to BaseType_t. Signed-off-by: Yuhui Zheng <[email protected]> * Update register test for ATmega4809 - to cover r28, r29, r31. - call public API taskYIELD() instead of portYIELD(). * Update ATmega4809 readme.md to include info for serial port setup, and minor wording fix. Co-authored-by: Alexandru Niculae - M17336 <[email protected]> Co-authored-by: S.Burch <[email protected]> Co-authored-by: RichardBarry <[email protected]> Co-authored-by: Hein Tibosch <[email protected]> Co-authored-by: Hein Tibosch <[email protected]> Co-authored-by: Nathan Chong <[email protected]> Co-authored-by: Gaurav-Aggarwal-AWS <[email protected]> Co-authored-by: Yuhui Zheng <[email protected]> Co-authored-by: Carl Lundin <[email protected]> Co-authored-by: Alexandru Niculae - M17336 <[email protected]>
* Add fail-safes to FreeRTOS_Socket.c * Use all 'pd' errors * Correction after Hein's comments * Correction after Hein's comments v2 * Changes after Hein's comments * Update after Gary's comments
* Updated version number of .[ch] files * Updated History.txt * Updated manifest.yml file * Updated the Kernel submodule and manifest.yml entry * Remove trailing whitespace * Update version number to V2.3.2 of remaining files
* Add fail-safes to FreeRTOS_Socket.c * Use all 'pd' errors * Correction after Hein's comments * Correction after Hein's comments v2 * Changes after Hein's comments * Update after Gary's comments
4 participants
Description
This PR adds changes to the FreeRTOS_Sockets.c to form a base for the Full-TCP test suite migration to FreeRTOS/FreeRTOS repository. The source is modified to add some checks in appropriate places such as
NULLchecking before dereferencing a pointer, defensive coding by assigning an error code to the return variable etc.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.