Skip to content
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

Missing null checks on PacketBuffer exhaustions in lwIP #22262

Closed
tcarmelveilleux opened this issue Aug 30, 2022 · 2 comments · Fixed by #22274
Closed

Missing null checks on PacketBuffer exhaustions in lwIP #22262

tcarmelveilleux opened this issue Aug 30, 2022 · 2 comments · Fixed by #22274

Comments

@tcarmelveilleux
Copy link
Contributor

Problem

Found during TC-RR-1.1 testing.

When we don't have enough memory to allocate a PacketBuffer handle, we return an empty packetbuffer handle:

    if (lPacket == nullptr)
    {
        ChipLogError(chipSystemLayer, "PacketBuffer: pool EMPTY.");
        return PacketBufferHandle();
    }

This requires every client to check for .IsNull().

Logs seen:

20:10:17:608] <0x1b>[0;31mE (63875) chip[DIS]: Insufficient parsers to process all SRV entries.<0x1b>[0m
[20:10:18:270] <0x1b>[0;32mI (64535) chip[EM]: Received message of type 0x2 with protocolId (0, 1) and MessageCounter:44186791 on exchange 33124r<0x1b>[0m
[20:10:18:293] <0x1b>[0;32mI (64545) chip[IN]: Prepared secure message 0x3ffca6c4 to 0x000000000001B669 (3)  of type 0x5 and protocolId (0, 1) on exchange 33124r with MessageCounter:126864535.<0x1b>[0m
[20:10:18:293] <0x1b>[0;31mE (64565) chip[CSL]: PacketBuffer: pool EMPTY.<0x1b>[0m
[20:10:18:293] <0x1b>[0;31mE (64565) chip[IN]: Cannot copy received pbuf of size 342<0x1b>[0m
[20:10:18:315] <0x1b>[0;32mI (64565) chip[IN]: Sending encrypted msg 0x3ffca6c4 with MessageCounter:126864535 to 0x000000000001B669 (3) at monotonic time: 000000000000F991 msec<0x1b>[0m
[20:10:18:315] <0x1b>[0;31mE (64565) chip[CSL]: PacketBuffer: pool EMPTY.<0x1b>[0m
[20:10:18:340] <0x1b>[0;31mE (64575) chip[CSL]: PacketBuffer: pool EMPTY.<0x1b>[0m
[20:10:18:340] <0x1b>[0;31mE (64595) chip[DMG]: <RE> Error sending out report data with b!<0x1b>[0m
[20:10:18:340] <0x1b>[0;31mE (64595) chip[IN]: Cannot copy received pbuf of size 510<0x1b>[0m
[20:10:18:340] <0x1b>[0;31mE (64605) chip[CSL]: PacketBuffer: pool EMPTY.<0x1b>[0m
[20:10:18:359] <0x1b>[0;31mE (64615) chip[IN]: Cannot copy received pbuf of size 510<0x1b>[0m
[20:10:18:359] <0x1b>[0;32mI (64615) chip[EM]: Received message of type 0x2 with protocolId (0, 1) and MessageCounter:44186791 on exchange 33124r<0x1b>[0m
[20:10:18:359] <0x1b>[0;31mE (64625) chip[CSL]: PacketBuffer: pool EMPTY.<0x1b>[0m
[20:10:18:382] <0x1b>[0;31mE (64635) chip[IN]: Cannot copy received pbuf of size 510<0x1b>[0m
[20:10:18:382] <0x1b>[0;31mE (64635) chip[CSL]: PacketBuffer: pool EMPTY.<0x1b>[0m
[20:10:18:382] <0x1b>[0;31mE (64645) chip[CSL]: PacketBuffer: pool EMPTY.<0x1b>[0m
[20:10:18:382] <0x1b>[0;31mE (64645) chip[CSL]: PacketBuffer: pool EMPTY.<0x1b>[0m
[20:10:18:404] <0x1b>[0
[20:10:18:404] abort() was called at PC 0x40133be0 on core 0
[20:10:18:404] 
[20:10:18:404] 

^^^ There were several pool EMPTY calls prior to a crash.

This would seem to indicate several places did correctly null-check, but some others not.

The PacketBufferWriteBase class says:


/**
 * BufferWriter backed by packet buffer.
 *
 * Typical use:
 *  @code
 *      PacketBufferWriter buf(maximumLength);
 *      if (buf.IsNull()) { return CHIP_ERROR_NO_MEMORY; }
 *      buf.Put(...);
 *      ...
 *      PacketBufferHandle handle = buf.Finalize();
 *      if (buf.IsNull()) { return CHIP_ERROR_BUFFER_TOO_SMALL; }
 *      // valid data
 *  @endcode
 */
template <class Writer>
class PacketBufferWriterBase : public Writer

It is clear that a null check needs to be done after alloc.

Audit of the code base has found several places where the null check is not done.

Proposed Solution

  • Add missing null checks where seen.
@tcarmelveilleux
Copy link
Contributor Author

Some of the sites apply to both lwIP and OpenThread UDPEndpoint, so actually this is not just a Wi-Fi or lwIP issue

tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue Aug 30, 2022
- Some callers of PacketBufferHandle::New did not null-check on
  failure to allocate. This is strongly linked to some crashes

Fixes project-chip#22262

This PR:
- Adds missing null checks required by API contract

Testing done:
- Unit tests still pass
- Conditions under which a crash previously occured no longer see
  a crash occur in manual testing against a real DUT
@andy31415
Copy link
Contributor

Patch acceptable: fixing a memory crash

tcarmelveilleux added a commit that referenced this issue Aug 31, 2022
- Some callers of PacketBufferHandle::New did not null-check on
  failure to allocate. This is strongly linked to some crashes

Fixes #22262

This PR:
- Adds missing null checks required by API contract

Testing done:
- Unit tests still pass
- Conditions under which a crash previously occured no longer see
  a crash occur in manual testing against a real DUT
isiu-apple pushed a commit to isiu-apple/connectedhomeip that referenced this issue Sep 16, 2022
…p#22274)

- Some callers of PacketBufferHandle::New did not null-check on
  failure to allocate. This is strongly linked to some crashes

Fixes project-chip#22262

This PR:
- Adds missing null checks required by API contract

Testing done:
- Unit tests still pass
- Conditions under which a crash previously occured no longer see
  a crash occur in manual testing against a real DUT
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants