-Warray-bounds report

[Exactlywb]

Hi! I've built ppp using -O2 -Werror=array-bounds and got an error:

discovery.c: In function ‘sendPADI’:
discovery.c:324:12: error: array subscript ‘PPPoETag {aka struct PPPoETagStruct}[0]’ is partly outside array bounds of ‘PPPoEPacket[1]’ {aka ‘struct PPPoEPacketStruct[1]’} [-Werror=array-bounds]
  324 |         svc->type = TAG_SERVICE_NAME;
      |            ^~
discovery.c:297:17: note: while referencing ‘packet’
  297 |     PPPoEPacket packet;
      |                 ^~~~~~
discovery.c:325:12: error: array subscript ‘PPPoETag {aka struct PPPoETagStruct}[0]’ is partly outside array bounds of ‘PPPoEPacket[1]’ {aka ‘struct PPPoEPacketStruct[1]’} [-Werror=array-bounds]
  325 |         svc->length = htons(namelen);
      |            ^~
discovery.c:297:17: note: while referencing ‘packet’
  297 |     PPPoEPacket packet;
      |                 ^~~~~~
discovery.c: In function ‘sendPADR’:
discovery.c:510:8: error: array subscript ‘PPPoETag {aka struct PPPoETagStruct}[0]’ is partly outside array bounds of ‘PPPoEPacket[1]’ {aka ‘struct PPPoEPacketStruct[1]’} [-Werror=array-bounds]
  510 |     svc->type = TAG_SERVICE_NAME;
      |        ^~
discovery.c:489:17: note: while referencing ‘packet’
  489 |     PPPoEPacket packet;
      |                 ^~~~~~
discovery.c:511:8: error: array subscript ‘PPPoETag {aka struct PPPoETagStruct}[0]’ is partly outside array bounds of ‘PPPoEPacket[1]’ {aka ‘struct PPPoEPacketStruct[1]’} [-Werror=array-bounds]
  511 |     svc->length = htons(namelen);
      |        ^~
discovery.c:489:17: note: while referencing ‘packet’
  489 |     PPPoEPacket packet;
      |                 ^~~~~~

Let's see truncated code:

PPPoEPacket packet;
PPPoETag *svc = (PPPoETag *) (&packet.payload);

As I understand sizeof (PPPoETag) = 1512, but sizeof (packet.payload) = 1508. It looks like wrong type cast.

Moreover, I want to point out that such type casts may provoke strict aliasing rule violation.

I hope my report will be useful for you.

Thanks

[enaess]

@pali Seems like an area you have submitted patches in. What do you think?

[pali]

There is check CHECK_ROOM(cursor, packet.payload, plen); prior using svc->type. So looks like it is safe.

[Exactlywb]

Yes, you are right: in this way there's (probably) no way to write more than 1506 bytes into PPPoETag so it looks safe. Thank you for the quick response.

[pali]

Anyway, I agree that code would be written better to avoid all those casting and to avoid those warnings. But the whole ppp project is old, based on old codebase, which means it needs lot of improvements. Anybody is welcome in such fixups / changes.