[PATCH v8] fix issues reported by GCC address sanitizer #1964

FanHong674 · 2023-12-05T05:59:31Z

Enable address sanitizer and fix bugs found by it.

configure.ac

test/validation/api/traffic_mngr/traffic_mngr.c

platform/linux-generic/pktio/pcap.c

helper/test/table.c

MatiasElo · 2023-12-07T16:03:14Z

It would be good to add address sanitizer also to CI. The test could be added for example to '--enable-deprecated --enable-helper-deprecated --enable-debug=full' conf in Run job.

configure.ac

example/ipsec_crypto/odp_ipsec_stream.c

example/ipsec_api/odp_ipsec.c

JereLeppanen · 2024-03-25T15:40:44Z

I took the liberty of updating this PR with all the comments processed, commit comments cleaned up a bit, and a couple of new warnings fixed (ML).

JereLeppanen · 2024-03-25T18:07:29Z

v4:

Fix buffer overrun in pktio/dpdk.

JereLeppanen · 2024-03-29T08:37:19Z

v4:

Rebase.
Fix typo in a commit comment.

test/validation/api/traffic_mngr/traffic_mngr.c

example/classifier/odp_classifier.c

platform/linux-generic/pktio/pcap.c

example/ipsec_crypto/odp_ipsec_stream.c

helper/test/table.c

.github/workflows/ci-pipeline.yml

JereLeppanen · 2024-04-08T13:31:59Z

All comments processed in v6.

JannePeltonen · 2024-04-11T07:58:31Z

helper/test/table.c

-	char ip_addr1[] = "12345678";
-	char ip_addr2[] = "11223344";
-	char ip_addr3[] = "55667788";
+	char key1[] = "1234";


The commit message is now out-of-date since the code no longer contains any comment about key format. I think the real reason for this commit is to fix the incorrect key length passed to the table creation function.

Currently the key length passed to f_create() is smaller than the array, but I wouldn't necessarily call that "incorrect".

This commit doesn't fix any functional problem, and is in that sense totally optional. The real reason for it is only to hopefully avoid some confusion.

I'll add the same in some form in the commit comment, which is otherwise correct, as far as I can tell, it's just missing the reason why the commit was made.

The code was incorrect (or obfuscated, if you prefer) in the sense that if the 8-byte ipaddr strings did not differ from each other in the first 4 bytes, things did not work as intended. This commit fixes that.

JannePeltonen · 2024-04-11T08:00:28Z

example/classifier/odp_classifier.c

@@ -783,6 +783,9 @@ int main(int argc, char *argv[])
 	if (odp_pool_destroy(pool))
 		ODPH_ERR("err: odp_pool_destroy error\n");

+	if (args->if_name)
+		free(args->if_name);


A very minor thing and a matter of taste, I guess, but the 'if' is redundant since free(0) is a no-op.

JannePeltonen · 2024-04-11T08:05:12Z

test/performance/odp_l2fwd_run.sh

 	LOG=odp_l2fwd_tmp.log

 	# Max 2 workers
-	$STDBUF odp_l2fwd${EXEEXT} -i $IF1,$IF2 -m 0 -t 5 -c 2 | tee $LOG


The commit message says: "Using stdbuf is only necessary when something in the output is being waited on, with e.g. tail -f. That's not the case here, so remove the unnecessary usage."

I am not sure the use of stdbuf here is unnecessary. When running the script in an interactive shell, the stfbuf causes the pps printouts to appear as soons as available, not after the scrip has run. So there is some benefit there. I am not sure if it outweighs the trouble though, so I am not sure what to think of this commit.

In some test scripts the script starts a task to the background and then waits for something in the background task's output. In those cases stdbuf is necessary for the script to work, but that is not the case here, hence "unnecessary". But anyway, commit comment reworded in v7.

JereLeppanen · 2024-04-11T14:35:10Z

All comments processed in v7.

Add '__odr_asan' to the legal global symbol list, for GCC address sanitizer. Signed-off-by: Fan Hong <fan.hong@arm.com> Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com> Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com> Reviewed-by: Matias Elo <matias.elo@nokia.com>

The payload_data array is too small for packets with small headers. Fix the MAX_PAYLOAD constant by calculating its value from packet size and smallest used header size. Buffer overflow reported by GCC address sanitizer. Also, check the requested payload size against MAX_PAYLOAD in make_pkt(). Signed-off-by: Fan Hong <fan.hong@arm.com> Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com> Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com> Reviewed-by: Matias Elo <matias.elo@nokia.com>

Free if_name, which is allocated using malloc(). Memory leak reported by GCC address sanitizer. Also, remove the conditional from another free() call, since free(NULL) is a no-op. Signed-off-by: Fan Hong <fan.hong@arm.com> Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com> Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com> Reviewed-by: Matias Elo <matias.elo@nokia.com>

Free the compiled bpf_program once it has been set. Memory leak reported by GCC address sanitizer. Signed-off-by: Fan Hong <fan.hong@arm.com> Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com> Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com> Reviewed-by: Matias Elo <matias.elo@nokia.com>

Free memory allocated for interface names at program termination. Memory leak reported by GCC address sanitizer. Signed-off-by: Fan Hong <fan.hong@arm.com> Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com> Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com> Reviewed-by: Matias Elo <matias.elo@nokia.com>

Free the dynamically allocated interface name pointer array and the string containing the interface names. Memory leak reported by GCC address sanitizer. Signed-off-by: Fan Hong <fan.hong@arm.com> Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com> Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com> Reviewed-by: Matias Elo <matias.elo@nokia.com>

GCC -fsanitize=address reports stack-buffer-overflow, because in this test the value size is 16 bytes, while the mac_addr*[] arrays used as values are only 13 bytes. Fix by using the array size as the value size. Remove the comment about ARP, since it could be misleading, and rename the mac_addr* variables to value*. Also fix incorrect comment in odph_hash_node. Signed-off-by: Fan Hong <fan.hong@arm.com> Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com> Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com> Reviewed-by: Matias Elo <matias.elo@nokia.com>

When the example digit csv is read into memory, reserve and zero the terminating null character. Buffer overrun reported by GCC address sanitizer. Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com> Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com> Reviewed-by: Matias Elo <matias.elo@nokia.com>

Add missing odp_ml_model_destroy() call. Memory leaks reported by GCC address sanitizer. Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com> Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com> Reviewed-by: Matias Elo <matias.elo@nokia.com>

Reduce the size of the arrays used as keys in this test down to 5 bytes, use the array size as the table key size, and rename the arrays from ip_addr* to key*. This commit doesn't fix any functional problem, it only makes the code slightly less confusing. Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com> Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com> Reviewed-by: Matias Elo <matias.elo@nokia.com>

Stdbuf interferes with address sanitizer: ==670682==ASan runtime does not come first in initial library list; you should either link runtime to your application or manually preload it with LD_PRELOAD. Using LD_PRELOAD does get around that problem, but it's better to not use stdbuf in the first place. There doesn't appear to be any specific need to use stdbuf here, so remove it. Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com> Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com> Reviewed-by: Matias Elo <matias.elo@nokia.com>

Reserve space for an extra character in mem_str[] so that sprintf() can print the null character there, even though we don't actually need it. Buffer overrun reported by GCC address sanitizer. Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com> Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com> Reviewed-by: Matias Elo <matias.elo@nokia.com>

Add a job that compiles using GCC address sanitizer and runs test cases, on Ubuntu 22.04. Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com> Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com> Reviewed-by: Matias Elo <matias.elo@nokia.com>

odpbuild changed the title ~~Add a build option for address sanitizer, with subsequent memory bugs fixed~~ [PATCH v1] Add a build option for address sanitizer, with subsequent memory bugs fixed Dec 5, 2023

FanHong674 force-pushed the local/asan branch from fc4c3ae to 9e02a25 Compare December 5, 2023 06:49

odpbuild changed the title ~~[PATCH v1] Add a build option for address sanitizer, with subsequent memory bugs fixed~~ [PATCH v2] Add a build option for address sanitizer, with subsequent memory bugs fixed Dec 5, 2023

MatiasElo reviewed Dec 7, 2023

View reviewed changes

JannePeltonen requested changes Dec 11, 2023

View reviewed changes

JereLeppanen mentioned this pull request Mar 4, 2024

[PATCH v1] fix address sanitizer findings #2027

Closed

JereLeppanen force-pushed the local/asan branch from 9e02a25 to d327d45 Compare March 25, 2024 15:36

odpbuild changed the title ~~[PATCH v2] Add a build option for address sanitizer, with subsequent memory bugs fixed~~ [PATCH v3] Add a build option for address sanitizer, with subsequent memory bugs fixed Mar 25, 2024

JereLeppanen force-pushed the local/asan branch from d327d45 to b09383c Compare March 25, 2024 18:05

odpbuild changed the title ~~[PATCH v3] Add a build option for address sanitizer, with subsequent memory bugs fixed~~ [PATCH v4] Add a build option for address sanitizer, with subsequent memory bugs fixed Mar 25, 2024

JereLeppanen changed the title ~~[PATCH v4] Add a build option for address sanitizer, with subsequent memory bugs fixed~~ [PATCH v4] fix issues reported by GCC address sanitizer Mar 25, 2024

JereLeppanen mentioned this pull request Mar 28, 2024

[PATCH v5] fix issues reported by GCC undefined sanitizer #2044

Merged

JereLeppanen force-pushed the local/asan branch from b09383c to 43e4d1f Compare March 29, 2024 08:33

odpbuild changed the title ~~[PATCH v4] fix issues reported by GCC address sanitizer~~ [PATCH v5] fix issues reported by GCC address sanitizer Mar 29, 2024

JannePeltonen requested changes Apr 8, 2024

View reviewed changes

JereLeppanen force-pushed the local/asan branch from 43e4d1f to 06498ba Compare April 8, 2024 13:30

odpbuild changed the title ~~[PATCH v5] fix issues reported by GCC address sanitizer~~ [PATCH v6] fix issues reported by GCC address sanitizer Apr 8, 2024

JannePeltonen reviewed Apr 11, 2024

View reviewed changes

JereLeppanen force-pushed the local/asan branch from 06498ba to 6423e0c Compare April 11, 2024 14:29

odpbuild changed the title ~~[PATCH v6] fix issues reported by GCC address sanitizer~~ [PATCH v7] fix issues reported by GCC address sanitizer Apr 11, 2024

JannePeltonen approved these changes Apr 12, 2024

View reviewed changes

FanHong674 and others added 12 commits April 12, 2024 10:27

github_ci: add address sanitizier run

9af4449

Add a job that compiles using GCC address sanitizer and runs test cases, on Ubuntu 22.04. Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com> Reviewed-by: Janne Peltonen <janne.peltonen@nokia.com> Reviewed-by: Matias Elo <matias.elo@nokia.com>

JereLeppanen force-pushed the local/asan branch from 6423e0c to 9af4449 Compare April 12, 2024 07:29

odpbuild changed the title ~~[PATCH v7] fix issues reported by GCC address sanitizer~~ [PATCH v8] fix issues reported by GCC address sanitizer Apr 12, 2024

MatiasElo enabled auto-merge (rebase) April 12, 2024 07:36

MatiasElo merged commit 7321f30 into OpenDataPlane:master Apr 12, 2024
172 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PATCH v8] fix issues reported by GCC address sanitizer #1964

[PATCH v8] fix issues reported by GCC address sanitizer #1964

FanHong674 commented Dec 5, 2023

MatiasElo commented Dec 7, 2023

JereLeppanen commented Mar 25, 2024

JereLeppanen commented Mar 25, 2024

JereLeppanen commented Mar 29, 2024

JereLeppanen commented Apr 8, 2024

JannePeltonen Apr 11, 2024

JereLeppanen Apr 11, 2024

JannePeltonen Apr 12, 2024

JannePeltonen Apr 11, 2024

JannePeltonen Apr 11, 2024

JereLeppanen Apr 11, 2024

JereLeppanen commented Apr 11, 2024

[PATCH v8] fix issues reported by GCC address sanitizer #1964

[PATCH v8] fix issues reported by GCC address sanitizer #1964

Conversation

FanHong674 commented Dec 5, 2023

MatiasElo commented Dec 7, 2023

JereLeppanen commented Mar 25, 2024

JereLeppanen commented Mar 25, 2024

JereLeppanen commented Mar 29, 2024

JereLeppanen commented Apr 8, 2024

JannePeltonen Apr 11, 2024

Choose a reason for hiding this comment

JereLeppanen Apr 11, 2024

Choose a reason for hiding this comment

JannePeltonen Apr 12, 2024

Choose a reason for hiding this comment

JannePeltonen Apr 11, 2024

Choose a reason for hiding this comment

JannePeltonen Apr 11, 2024

Choose a reason for hiding this comment

JereLeppanen Apr 11, 2024

Choose a reason for hiding this comment

JereLeppanen commented Apr 11, 2024