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

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

Merged
merged 13 commits into from
Apr 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions .github/workflows/ci-pipeline.yml
Original file line number Diff line number Diff line change
Expand Up @@ -445,3 +445,21 @@ jobs:
-e CONF="${CONF}" $CONTAINER_NAMESPACE/odp-ci-${OS}-${ARCH}-dpdk_21.11 /odp/scripts/ci/check.sh
- if: ${{ failure() }}
uses: ./.github/actions/run-failure-log

Run_sanitizer:
runs-on: ubuntu-20.04
env:
OS: ubuntu_22.04
strategy:
fail-fast: false
matrix:
flags: ['-fsanitize=address']
steps:
- uses: actions/checkout@v4
- run: sudo docker run -i -v `pwd`:/odp --privileged --shm-size 8g -e CC="${CC}" -e ARCH="${ARCH}"
-e CFLAGS="-O0 -g -Wno-error ${{matrix.flags}}"
JannePeltonen marked this conversation as resolved.
Show resolved Hide resolved
-e CXXFLAGS="-O0 -g -Wno-error ${{matrix.flags}}"
-e LDFLAGS="-g ${{matrix.flags}}"
$CONTAINER_NAMESPACE/odp-ci-${OS}-${ARCH} /odp/scripts/ci/check.sh
- if: ${{ failure() }}
uses: ./.github/actions/run-failure-log
6 changes: 3 additions & 3 deletions example/classifier/odp_classifier.c
Original file line number Diff line number Diff line change
Expand Up @@ -783,6 +783,8 @@ int main(int argc, char *argv[])
if (odp_pool_destroy(pool))
ODPH_ERR("err: odp_pool_destroy error\n");

free(args->if_name);

args_error:
odp_shm_free(shm);

Expand Down Expand Up @@ -1281,9 +1283,7 @@ static int parse_args(int argc, char *argv[], appl_args_t *appl_args)

if (ret) {
usage();

if (appl_args->if_name)
free(appl_args->if_name);
free(appl_args->if_name);
}

/* reset optind from the getopt lib */
Expand Down
7 changes: 4 additions & 3 deletions example/ipsec_api/odp_ipsec.c
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
#include <odp_ipsec_stream.h>
JannePeltonen marked this conversation as resolved.
Show resolved Hide resolved
#else
static void init_stream_db(void) {}
static void deinit_stream_db(void) {}
static void resolve_stream_db(void) {}
static int create_stream_db_inputs(void)
{
Expand Down Expand Up @@ -1154,9 +1155,9 @@ main(int argc, char *argv[])
shm = odp_shm_lookup("shm_sp_db");
if (odp_shm_free(shm) != 0)
ODPH_ERR("Error: shm free shm_sp_db failed\n");
shm = odp_shm_lookup("stream_db");
if (odp_shm_free(shm) != 0)
ODPH_ERR("Error: shm free stream_db failed\n");

deinit_stream_db();

if (odp_shm_free(global->shm)) {
ODPH_ERR("Error: shm free global data failed\n");
exit(EXIT_FAILURE);
Expand Down
7 changes: 4 additions & 3 deletions example/ipsec_crypto/odp_ipsec.c
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
#include <odp_ipsec_stream.h>
#else
static void init_stream_db(void) {}
static void deinit_stream_db(void) {}
static void resolve_stream_db(void) {}
static int create_stream_db_inputs(void)
{
Expand Down Expand Up @@ -1437,9 +1438,9 @@ main(int argc, char *argv[])
shm = odp_shm_lookup("shm_sp_db");
if (odp_shm_free(shm) != 0)
ODPH_ERR("Error: shm free shm_sp_db failed\n");
shm = odp_shm_lookup("stream_db");
if (odp_shm_free(shm) != 0)
ODPH_ERR("Error: shm free stream_db failed\n");

deinit_stream_db();

if (odp_shm_free(global->shm)) {
ODPH_ERR("Error: shm free global data failed\n");
exit(EXIT_FAILURE);
Expand Down
25 changes: 24 additions & 1 deletion example/ipsec_crypto/odp_ipsec_stream.c
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,14 @@ typedef struct ODP_PACKED stream_pkt_hdr_s {
uint8_t data[]; /**< Incrementing data stream */
} stream_pkt_hdr_t;

static const char *shm_name = "stream_db";
stream_db_t *stream_db;

void init_stream_db(void)
{
odp_shm_t shm;

shm = odp_shm_reserve("stream_db",
shm = odp_shm_reserve(shm_name,
sizeof(stream_db_t),
ODP_CACHE_LINE_SIZE,
0);
Expand All @@ -65,6 +66,28 @@ void init_stream_db(void)
memset(stream_db, 0, sizeof(*stream_db));
}

void deinit_stream_db(void)
{
stream_db_entry_t *stream = NULL;

for (stream = stream_db->list; NULL != stream; stream = stream->next) {
free(stream->input.intf);
free(stream->output.intf);
}

odp_shm_t shm = odp_shm_lookup(shm_name);
JannePeltonen marked this conversation as resolved.
Show resolved Hide resolved

if (shm == ODP_SHM_INVALID) {
ODPH_ERR("Error: shared mem not found.\n");
exit(EXIT_FAILURE);
}

if (odp_shm_free(shm)) {
ODPH_ERR("Error: shared mem free failed.\n");
exit(EXIT_FAILURE);
}
}

int create_stream_db_entry(char *input)
{
int pos = 0;
Expand Down
7 changes: 5 additions & 2 deletions example/ipsec_crypto/odp_ipsec_stream.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,14 @@ typedef struct stream_db_entry_s {
uint32_t verified; /**< Number successfully verified */
const EVP_MD *evp_md; /**< Digest method */
struct {
const char *intf; /**< Input interface name */
char *intf; /**< Input interface name */
odp_pktio_t pktio; /**< Input PktI/O interface */
uint32_t ah_seq; /**< AH sequence number if present */
uint32_t esp_seq; /**< ESP sequence number if present */
ipsec_cache_entry_t *entry; /**< IPsec to apply on input */
} input;
struct {
const char *intf; /**< Output interface name */
char *intf; /**< Output interface name */
odp_pktio_t pktio; /**< Output PktI/O interface */
ipsec_cache_entry_t *entry; /**t IPsec to verify on output */
} output;
Expand All @@ -58,6 +58,9 @@ extern stream_db_t *stream_db;
/** Initialize stream database global control structure */
void init_stream_db(void);

/** Deinitialize stream database global control structure */
void deinit_stream_db(void);

/**
* Create an stream DB entry
*
Expand Down
3 changes: 3 additions & 0 deletions example/simple_pipeline/odp_simple_pipeline.c
Original file line number Diff line number Diff line change
Expand Up @@ -922,6 +922,9 @@ int main(int argc, char **argv)

odph_thread_join(thr_tbl, num_threads);

free(global->appl.if_names);
free(global->appl.if_str);

if (odp_pktio_close(global->if0)) {
printf("Error: failed to close interface %s\n", argv[1]);
exit(EXIT_FAILURE);
Expand Down
2 changes: 1 addition & 1 deletion helper/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ __LIB__libodphelper_la_LIBADD += $(LIBCLI_LIBS)

lib_LTLIBRARIES = $(LIB)/libodphelper.la

CHECK_GLOBALS_REGEX = " (odph_|_deprecated_odph_)"
CHECK_GLOBALS_REGEX = " (odph_|_deprecated_odph_|__odr_asan)"

TESTS_ENVIRONMENT = \
LIBTOOL="$(LIBTOOL)" \
Expand Down
2 changes: 1 addition & 1 deletion helper/hashtable.c
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ typedef struct odph_hash_node {
* Its length is key_size + value_size,
* suppose key_size = m; value_size = n;
* its structure is like:
* k_byte1 k_byte2...k_byten v_byte1...v_bytem
* k_byte1 k_byte2...k_bytem v_byte1...v_byten
*/
char content[];
} odph_hash_node;
Expand Down
50 changes: 17 additions & 33 deletions helper/test/table.c
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,6 @@
#include <odp_api.h>
#include <odp/helper/odph_api.h>

/**
* Address Resolution Protocol (ARP)
* Description: Once a route has been identified for an IP packet (so the
* output interface and the IP address of the next hop station are known),
* the MAC address of the next hop station is needed in order to send this
* packet onto the next leg of the journey towards its destination
* (as identified by its destination IP address). The MAC address of the next
* hop station becomes the destination MAC address of the outgoing
* Ethernet frame.
* Hash table name: ARP table
* Number of keys: Thousands
* Key format: The pair of (Output interface, Next Hop IP address),
* which is typically 5 bytes for IPv4 and 17 bytes for IPv6.
* value (data): MAC address of the next hop station (6 bytes).
*/

int main(int argc ODP_UNUSED, char *argv[] ODP_UNUSED)
{
odp_instance_t instance;
Expand All @@ -29,13 +13,13 @@ int main(int argc ODP_UNUSED, char *argv[] ODP_UNUSED)
odph_table_t tmp_tbl;
odph_table_ops_t *test_ops;
char tmp[32];
char ip_addr1[] = "12345678";
char ip_addr2[] = "11223344";
char ip_addr3[] = "55667788";
char mac_addr1[] = "0A1122334401";
char mac_addr2[] = "0A1122334402";
char mac_addr3[] = "0B4433221101";
char mac_addr4[] = "0B4433221102";
char key1[] = "1234";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

char key2[] = "1122";
char key3[] = "3344";
char value1[] = "0A1122334401";
char value2[] = "0A1122334402";
char value3[] = "0B4433221101";
char value4[] = "0B4433221102";

ret = odp_init_global(&instance, NULL, NULL);
if (ret != 0) {
Expand All @@ -51,49 +35,49 @@ int main(int argc ODP_UNUSED, char *argv[] ODP_UNUSED)
printf("test hash table:\n");
test_ops = &odph_hash_table_ops;

table = test_ops->f_create("test", 2, 4, 16);
table = test_ops->f_create("test", 2, sizeof(key1), sizeof(value1));
if (table == NULL) {
printf("table create fail\n");
return -1;
}
ret += test_ops->f_put(table, &ip_addr1, mac_addr1);
ret += test_ops->f_put(table, &key1, value1);

ret += test_ops->f_put(table, &ip_addr2, mac_addr2);
ret += test_ops->f_put(table, &key2, value2);

ret += test_ops->f_put(table, &ip_addr3, mac_addr3);
ret += test_ops->f_put(table, &key3, value3);

if (ret != 0) {
printf("put value fail\n");
return -1;
}

ret = test_ops->f_get(table, &ip_addr1, &tmp, 32);
ret = test_ops->f_get(table, &key1, &tmp, 32);
if (ret != 0) {
printf("get value fail\n");
return -1;
}
printf("\t1 get '123' tmp = %s,\n", tmp);

ret = test_ops->f_put(table, &ip_addr1, mac_addr4);
ret = test_ops->f_put(table, &key1, value4);
if (ret != 0) {
printf("repeat put value fail\n");
return -1;
}

ret = test_ops->f_get(table, &ip_addr1, &tmp, 32);
if (ret != 0 || strcmp(tmp, mac_addr4) != 0) {
ret = test_ops->f_get(table, &key1, &tmp, 32);
if (ret != 0 || memcmp(tmp, value4, sizeof(value4)) != 0) {
printf("get value fail\n");
return -1;
}

printf("\t2 repeat get '123' value = %s\n", tmp);

ret = test_ops->f_remove(table, &ip_addr1);
ret = test_ops->f_remove(table, &key1);
if (ret != 0) {
printf("remove value fail\n");
return -1;
}
ret = test_ops->f_get(table, &ip_addr1, tmp, 32);
ret = test_ops->f_get(table, &key1, tmp, 32);
if (ret == 0) {
printf("remove value fail actually\n");
return -1;
Expand Down
2 changes: 1 addition & 1 deletion platform/linux-generic/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,7 @@ if ODP_PKTIO_PCAP
__LIB__libodp_linux_la_LIBADD += $(PCAP_LIBS)
endif

CHECK_GLOBALS_REGEX = " (odp_|_odp_|_deprecated_odp_|miniz_|mz_|tdefl_|tinfl_|mp_hdlr_init_odp_pool_ops)"
CHECK_GLOBALS_REGEX = " (odp_|_odp_|_deprecated_odp_|miniz_|mz_|tdefl_|tinfl_|mp_hdlr_init_odp_pool_ops|__odr_asan)"

TESTS_ENVIRONMENT = \
LIBTOOL="$(LIBTOOL)" \
Expand Down
4 changes: 2 additions & 2 deletions platform/linux-generic/example/ml/mnist.c
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ static int read_digit_csv(const char *file_name, uint8_t *expected_digit, float
size = ftell(digit_file);
rewind(digit_file);

tmp = malloc(size);
memset(tmp, 0, size);
tmp = malloc(size + 1);
memset(tmp, 0, size + 1);
num_elem = fread(tmp, size, 1, digit_file);

fclose(digit_file);
Expand Down
5 changes: 5 additions & 0 deletions platform/linux-generic/example/ml/model_explorer.c
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,11 @@ int main(int argc, char *argv[])

odp_ml_model_print(ml_model);

if (odp_ml_model_destroy(ml_model)) {
printf("odp_ml_model_destroy failed.\n");
ret = -1;
}

odp_term:
if (odp_term_local()) {
printf("Local term failed.\n");
Expand Down
2 changes: 1 addition & 1 deletion platform/linux-generic/pktio/dpdk.c
Original file line number Diff line number Diff line change
Expand Up @@ -1435,7 +1435,7 @@ static int dpdk_pktio_init(void)
if (numa_nodes <= 0)
numa_nodes = 1;

char mem_str[mem_str_len * numa_nodes];
char mem_str[mem_str_len * numa_nodes + 1];

for (i = 0; i < numa_nodes; i++)
sprintf(&mem_str[i * mem_str_len], "%d,", DPDK_MEMORY_MB);
Expand Down
2 changes: 2 additions & 0 deletions platform/linux-generic/pktio/pcap.c
Original file line number Diff line number Diff line change
Expand Up @@ -193,10 +193,12 @@ static int pcapif_promisc_mode_set(pktio_entry_t *pktio_entry,
}

if (pcap_setfilter(pcap->rx, &bpf) != 0) {
pcap_freecode(&bpf);
_ODP_ERR("failed to set promisc mode filter: %s\n", pcap_geterr(pcap->rx));
return -1;
}

pcap_freecode(&bpf);
pcap->promisc = enable;

return 0;
Expand Down
9 changes: 1 addition & 8 deletions test/performance/odp_l2fwd_run.sh
Original file line number Diff line number Diff line change
Expand Up @@ -71,17 +71,10 @@ run_l2fwd()

GEN_PID=$!

# this just turns off output buffering so that you still get periodic
# output while piping to tee, as long as stdbuf is available.
if [ "$(which stdbuf)" != "" ]; then
STDBUF="stdbuf -o 0"
else
STDBUF=
fi
LOG=odp_l2fwd_tmp.log

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

odp_l2fwd${EXEEXT} -i $IF1,$IF2 -m 0 -t 5 -c 2 | tee $LOG
ret=${PIPESTATUS[0]}

kill -2 ${GEN_PID}
Expand Down
10 changes: 1 addition & 9 deletions test/performance/odp_pktio_ordered_run.sh
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,7 @@ if [ ! -f ${PCAP_IN} ]; then
exit 1
fi

# This just turns off output buffering so that you still get periodic
# output while piping to tee, as long as stdbuf is available.
if [ "$(which stdbuf)" != "" ]; then
STDBUF="stdbuf -o 0"
else
STDBUF=
fi

$STDBUF ${TEST_DIR}/odp_pktio_ordered${EXEEXT} \
${TEST_DIR}/odp_pktio_ordered${EXEEXT} \
-i pcap:in=${PCAP_IN}:loops=$LOOPS,pcap:out=${PCAP_OUT} \
-t $DURATION | tee $LOG
ret=${PIPESTATUS[0]}
Expand Down
Loading