New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests malloc/realloc return values in MemoryDataStream->write #661

Merged
merged 3 commits into from Apr 28, 2016

Conversation

Projects
None yet
2 participants
@festlv
Contributor

festlv commented Mar 21, 2016

malloc/realloc function call return values were not checked and used as buffers which could lead to NULL pointer de-referencing crashes.

Additionally, TcpClient->send now fails when there was no free space in underlying memory stream.

@hreintke

This comment has been minimized.

Show comment
Hide comment
@hreintke

hreintke Mar 21, 2016

Contributor

@festlv :
Good catch.
Update to memorydatastream is OK.
On TCPClient. :
In the current implementation the side effect is that when closing the connection it will be detected as "not completely send data" because of the asyncTotalLen += len; .
After your update this is not updated -> asyncTotalLen == asyncTotalSent -> closed with "all OK'

Maybe should close the connection when data cannot be stored. The principle of TCP connection is a guaranteed transmission of data. Now we leave part of the data out.

Contributor

hreintke commented Mar 21, 2016

@festlv :
Good catch.
Update to memorydatastream is OK.
On TCPClient. :
In the current implementation the side effect is that when closing the connection it will be detected as "not completely send data" because of the asyncTotalLen += len; .
After your update this is not updated -> asyncTotalLen == asyncTotalSent -> closed with "all OK'

Maybe should close the connection when data cannot be stored. The principle of TCP connection is a guaranteed transmission of data. Now we leave part of the data out.

@festlv

This comment has been minimized.

Show comment
Hide comment
@festlv

festlv Mar 22, 2016

Contributor

I actually realized that MemoryDataStream update will leak memory if initial malloc succeeds but realloc fails- buf is NULL and won't be freed. Will update the PR later.

Regarding TcpClient- I see your point. However, my update does not break this guarantee- the data which has been accepted for sending is sent. If TcpClient fails to send data (send returns false), application layer knows it right away and can act accordingly. Otherwise, the current implementation will discard data and application will only learn about this later.

Contributor

festlv commented Mar 22, 2016

I actually realized that MemoryDataStream update will leak memory if initial malloc succeeds but realloc fails- buf is NULL and won't be freed. Will update the PR later.

Regarding TcpClient- I see your point. However, my update does not break this guarantee- the data which has been accepted for sending is sent. If TcpClient fails to send data (send returns false), application layer knows it right away and can act accordingly. Otherwise, the current implementation will discard data and application will only learn about this later.

@hreintke

This comment has been minimized.

Show comment
Hide comment
@hreintke

hreintke Apr 28, 2016

Contributor

Can you make a PR for SmingRTOS too ?

Contributor

hreintke commented Apr 28, 2016

Can you make a PR for SmingRTOS too ?

@hreintke hreintke merged commit 28e77e6 into SmingHub:develop Apr 28, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@hreintke hreintke removed the PR_Progress label Apr 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment