linux-test-project / ltp

Linux Test Project (mailing list: https://lists.linux.it/listinfo/ltp)
https://linux-test-project.readthedocs.io/
GNU General Public License v2.0
2.31k stars 1.01k forks source link

sctp/connectx: remove meaningless TEST3 #975

Closed Rtoax closed 2 years ago

Rtoax commented 2 years ago
test_1_to_1_connectx.c: In function ‘main’:
test_1_to_1_connectx.c:137:17: warning: array subscript ‘struct sockaddr[0]’ is partly outside array bounds of ‘unsigned char[15]’ [-Warray-bounds]
  137 |         tmp_addr->sa_family = AF_INET;
      |                 ^~
test_1_to_1_connectx.c:136:40: note: referencing an object of size 15 allocated by ‘malloc’
  136 |         tmp_addr = (struct sockaddr *) malloc(sizeof(struct sockaddr) - 1);
      |
metan-ucw commented 2 years ago

Looking at the code in questions the code is more broken than this. This is an attempt to check that the call returns EINVAL with invalid address and attempts to do that by allocating the structure short of one byte.

But this does not work at all, since in case of AF_INET the last eight bytes are padding anyways. I do not think that we can create invalid struct sockaddr_in in the first place, as far as I can tell we can only pass invalid sa_family which we do in next test. So all in all it looks like we should remove the TEST3 since it does not make any sense.

pevik commented 2 years ago

@Rtoax will you please send patch to remove TEST3 to our (LTP) mailing list and to linux-sctp@vger.kernel.org (preferred) or PR? If you're busy I can do it.

@metan-ucw is it serious enough to get it into the release? It has been broken long time ago: https://openqa.opensuse.org/tests/2732829#next_previous

metan-ucw commented 2 years ago

I guess that it can wait after we finalize the release, this has been broken for years.

Rtoax commented 2 years ago

Yep, done.

Rtoax commented 2 years ago

Delete TEST3 in this PR.

pevik commented 1 year ago

FYI I looked at the "upstream" repo used by kernel SCTP developers: https://github.com/sctp/lksctp-tools/blob/master/src/func_tests/test_1_to_1_connectx.c, they use

tmp_addr = (struct sockaddr *) malloc(sizeof(struct sockaddr));

instead of

tmp_addr = (struct sockaddr *) malloc(sizeof(struct sockaddr) - 1);

i.e. not using -1. to invalidate address. But I tested it on aarch64 and it fails the same way as our version:

test_1_to_1_events.c    1  TPASS  :  COMM_UP notification on client socket - SUCCESS
test_1_to_1_events.c    2  TPASS  :  COMM_UP notification on server socket - SUCCESS
test_1_to_1_events.c    3  TBROK  :  sctputil.c:241: Got a notification, expecting a datamsg
test_1_to_1_events.c    4  TBROK  :  sctputil.c:241: Remaining cases broken

This is a sign to me that even repo has some commits it's not that much maintained (see the discussion where SCTP tests should be hosted: https://lore.kernel.org/linux-sctp/YfpnVfrto4Elshy5@pevik/).

pevik commented 1 year ago

Although this failure is on older kernel (latest SLES), latest mainline stable works.