libosmocore[master]: tests/ussd: prevent uninitialized memory access
Patch Set 1: It's funny that the address sanitizer didn't catch this bug ;) -- To view, visit https://gerrit.osmocom.org/5934 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic31805b6a5a917dfc6284edba6ffdd21246ac20c Gerrit-PatchSet: 1 Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Vadim Yanitskiy Gerrit-HasComments: No
libosmocore[master]: tests/ussd: prevent uninitialized memory access
Patch Set 1: (1 comment) https://gerrit.osmocom.org/#/c/5934/1//COMMIT_MSG Commit Message: Line 16: value, that octets_written is. This then causes the and why is that? strlen() doesn't include any NUL byte, so why is it longer? -- To view, visit https://gerrit.osmocom.org/5934 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic31805b6a5a917dfc6284edba6ffdd21246ac20c Gerrit-PatchSet: 1 Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Vadim Yanitskiy Gerrit-HasComments: Yes
libosmocore[master]: tests/ussd: prevent uninitialized memory access
Patch Set 1: (1 comment) https://gerrit.osmocom.org/#/c/5934/1//COMMIT_MSG Commit Message: Line 16: value, that octets_written is. This then causes the > and why is that? strlen() doesn't include any NUL byte, so why is it longer Good question, I'll look closer and update commit msg. You can also check putting a debug printf ;) -- To view, visit https://gerrit.osmocom.org/5934 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic31805b6a5a917dfc6284edba6ffdd21246ac20c Gerrit-PatchSet: 1 Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Vadim Yanitskiy Gerrit-HasComments: Yes
libosmocore[master]: tests/ussd: prevent uninitialized memory access
Patch Set 2: > You can also check putting a debug printf That's actually good idea: you can add this printf and update expected test output to remove any doubts. -- To view, visit https://gerrit.osmocom.org/5934 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic31805b6a5a917dfc6284edba6ffdd21246ac20c Gerrit-PatchSet: 2 Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Max Gerrit-Reviewer: Vadim Yanitskiy Gerrit-HasComments: No
libosmocore[master]: tests/ussd: prevent uninitialized memory access
Patch Set 2: > > You can also check putting a debug printf > > That's actually good idea: you can add this printf and update > expected test output to remove any doubts. But unrelated to USSD testing at all ;) @Harald, the answer to this question is in the commit msg now. -- To view, visit https://gerrit.osmocom.org/5934 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic31805b6a5a917dfc6284edba6ffdd21246ac20c Gerrit-PatchSet: 2 Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Max Gerrit-Reviewer: Vadim Yanitskiy Gerrit-HasComments: No
libosmocore[master]: tests/ussd: prevent uninitialized memory access
Patch Set 2: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/5934 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic31805b6a5a917dfc6284edba6ffdd21246ac20c Gerrit-PatchSet: 2 Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Owner: Vadim Yanitskiy Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Max Gerrit-Reviewer: Vadim Yanitskiy Gerrit-HasComments: No
[PATCH] libosmocore[master]: tests/ussd: prevent uninitialized memory access
Review at https://gerrit.osmocom.org/5934
tests/ussd: prevent uninitialized memory access
Previously an incorrect length value was passed to both
gsm_7bit_decode_n_ussd() and gsm_7bit_encode_n_ussd()
functions during test_7bit_ussd() execution, due to:
octets_written = strlen(decoded);
The problem is that strlen() returns one-byte bigger
value, that octets_written is. This then causes the
uninitialized memory access.
Found using Valgrind:
Conditional jump or move depends on uninitialised value(s)
at 0x506DCCC: gsm_7bit_decode_n_ussd (gsm_utils.c:248)
by 0x40134B: test_7bit_ussd (ussd_test.c:104)
by 0x400F5D: main (ussd_test.c:161)
Conditional jump or move depends on uninitialised value(s)
at 0x506DBB7: gsm_7bit_decode_n_hdr (gsm_utils.c:220)
by 0x506DC9E: gsm_7bit_decode_n_ussd (gsm_utils.c:246)
by 0x40134B: test_7bit_ussd (ussd_test.c:104)
by 0x400F5D: main (ussd_test.c:161)
Conditional jump or move depends on uninitialised value(s)
at 0x506DBCB: gsm_septet_lookup (gsm_utils.c:153)
by 0x506DBCB: gsm_7bit_decode_n_hdr (gsm_utils.c:224)
by 0x506DC9E: gsm_7bit_decode_n_ussd (gsm_utils.c:246)
by 0x40134B: test_7bit_ussd (ussd_test.c:104)
by 0x400F5D: main (ussd_test.c:161)
Change-Id: Ic31805b6a5a917dfc6284edba6ffdd21246ac20c
---
M tests/ussd/ussd_test.c
1 file changed, 1 insertion(+), 2 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/34/5934/1
diff --git a/tests/ussd/ussd_test.c b/tests/ussd/ussd_test.c
index 8d88dbb..429c72d 100644
--- a/tests/ussd/ussd_test.c
+++ b/tests/ussd/ussd_test.c
@@ -90,8 +90,7 @@
OSMO_ASSERT(strcmp(encoded_hex, osmo_hexdump_nospc(coded,
octets_written)) == 0);
gsm_7bit_decode_n_ussd(decoded, sizeof(decoded), coded, octets_written
* 8 / 7);
- octets_written = strlen(decoded);
- printf("decoded = %s\n\n", osmo_hexdump((uint8_t *)decoded,
octets_written));
+ printf("decoded = %s\n\n", osmo_hexdump((uint8_t *)decoded,
strlen(decoded)));
OSMO_ASSERT(strncmp(text, decoded, strlen(text)) == 0);
OSMO_ASSERT(strcmp(appended_after_decode, decoded + strlen(text)) == 0);
--
To view, visit https://gerrit.osmocom.org/5934
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic31805b6a5a917dfc6284edba6ffdd21246ac20c
Gerrit-PatchSet: 1
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Vadim Yanitskiy
[PATCH] libosmocore[master]: tests/ussd: prevent uninitialized memory access
Hello Jenkins Builder,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/5934
to look at the new patch set (#2).
tests/ussd: prevent uninitialized memory access
Previously an incorrect length value was passed to both
gsm_7bit_decode_n_ussd() and gsm_7bit_encode_n_ussd()
functions during test_7bit_ussd() execution, due to:
octets_written = strlen(decoded);
The problem is that a 7-bit encoded string takes less memory
than its 8-bit equivalent. So, here strlen() returns one-byte
bigger value, that octets_written is. This then causes the
uninitialized memory access.
Found using Valgrind:
Conditional jump or move depends on uninitialised value(s)
at 0x506DCCC: gsm_7bit_decode_n_ussd (gsm_utils.c:248)
by 0x40134B: test_7bit_ussd (ussd_test.c:104)
by 0x400F5D: main (ussd_test.c:161)
Conditional jump or move depends on uninitialised value(s)
at 0x506DBB7: gsm_7bit_decode_n_hdr (gsm_utils.c:220)
by 0x506DC9E: gsm_7bit_decode_n_ussd (gsm_utils.c:246)
by 0x40134B: test_7bit_ussd (ussd_test.c:104)
by 0x400F5D: main (ussd_test.c:161)
Conditional jump or move depends on uninitialised value(s)
at 0x506DBCB: gsm_septet_lookup (gsm_utils.c:153)
by 0x506DBCB: gsm_7bit_decode_n_hdr (gsm_utils.c:224)
by 0x506DC9E: gsm_7bit_decode_n_ussd (gsm_utils.c:246)
by 0x40134B: test_7bit_ussd (ussd_test.c:104)
by 0x400F5D: main (ussd_test.c:161)
Change-Id: Ic31805b6a5a917dfc6284edba6ffdd21246ac20c
---
M tests/ussd/ussd_test.c
1 file changed, 1 insertion(+), 2 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/34/5934/2
diff --git a/tests/ussd/ussd_test.c b/tests/ussd/ussd_test.c
index 8d88dbb..429c72d 100644
--- a/tests/ussd/ussd_test.c
+++ b/tests/ussd/ussd_test.c
@@ -90,8 +90,7 @@
OSMO_ASSERT(strcmp(encoded_hex, osmo_hexdump_nospc(coded,
octets_written)) == 0);
gsm_7bit_decode_n_ussd(decoded, sizeof(decoded), coded, octets_written
* 8 / 7);
- octets_written = strlen(decoded);
- printf("decoded = %s\n\n", osmo_hexdump((uint8_t *)decoded,
octets_written));
+ printf("decoded = %s\n\n", osmo_hexdump((uint8_t *)decoded,
strlen(decoded)));
OSMO_ASSERT(strncmp(text, decoded, strlen(text)) == 0);
OSMO_ASSERT(strcmp(appended_after_decode, decoded + strlen(text)) == 0);
--
To view, visit https://gerrit.osmocom.org/5934
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic31805b6a5a917dfc6284edba6ffdd21246ac20c
Gerrit-PatchSet: 2
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Vadim Yanitskiy
Gerrit-Reviewer: Harald Welte
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Vadim Yanitskiy
[MERGED] libosmocore[master]: tests/ussd: prevent uninitialized memory access
Harald Welte has submitted this change and it was merged.
Change subject: tests/ussd: prevent uninitialized memory access
..
tests/ussd: prevent uninitialized memory access
Previously an incorrect length value was passed to both
gsm_7bit_decode_n_ussd() and gsm_7bit_encode_n_ussd()
functions during test_7bit_ussd() execution, due to:
octets_written = strlen(decoded);
The problem is that a 7-bit encoded string takes less memory
than its 8-bit equivalent. So, here strlen() returns one-byte
bigger value, that octets_written is. This then causes the
uninitialized memory access.
Found using Valgrind:
Conditional jump or move depends on uninitialised value(s)
at 0x506DCCC: gsm_7bit_decode_n_ussd (gsm_utils.c:248)
by 0x40134B: test_7bit_ussd (ussd_test.c:104)
by 0x400F5D: main (ussd_test.c:161)
Conditional jump or move depends on uninitialised value(s)
at 0x506DBB7: gsm_7bit_decode_n_hdr (gsm_utils.c:220)
by 0x506DC9E: gsm_7bit_decode_n_ussd (gsm_utils.c:246)
by 0x40134B: test_7bit_ussd (ussd_test.c:104)
by 0x400F5D: main (ussd_test.c:161)
Conditional jump or move depends on uninitialised value(s)
at 0x506DBCB: gsm_septet_lookup (gsm_utils.c:153)
by 0x506DBCB: gsm_7bit_decode_n_hdr (gsm_utils.c:224)
by 0x506DC9E: gsm_7bit_decode_n_ussd (gsm_utils.c:246)
by 0x40134B: test_7bit_ussd (ussd_test.c:104)
by 0x400F5D: main (ussd_test.c:161)
Change-Id: Ic31805b6a5a917dfc6284edba6ffdd21246ac20c
---
M tests/ussd/ussd_test.c
1 file changed, 1 insertion(+), 2 deletions(-)
Approvals:
Harald Welte: Looks good to me, approved
Jenkins Builder: Verified
diff --git a/tests/ussd/ussd_test.c b/tests/ussd/ussd_test.c
index 8d88dbb..429c72d 100644
--- a/tests/ussd/ussd_test.c
+++ b/tests/ussd/ussd_test.c
@@ -90,8 +90,7 @@
OSMO_ASSERT(strcmp(encoded_hex, osmo_hexdump_nospc(coded,
octets_written)) == 0);
gsm_7bit_decode_n_ussd(decoded, sizeof(decoded), coded, octets_written
* 8 / 7);
- octets_written = strlen(decoded);
- printf("decoded = %s\n\n", osmo_hexdump((uint8_t *)decoded,
octets_written));
+ printf("decoded = %s\n\n", osmo_hexdump((uint8_t *)decoded,
strlen(decoded)));
OSMO_ASSERT(strncmp(text, decoded, strlen(text)) == 0);
OSMO_ASSERT(strcmp(appended_after_decode, decoded + strlen(text)) == 0);
--
To view, visit https://gerrit.osmocom.org/5934
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic31805b6a5a917dfc6284edba6ffdd21246ac20c
Gerrit-PatchSet: 3
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Vadim Yanitskiy
Gerrit-Reviewer: Harald Welte
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max
Gerrit-Reviewer: Vadim Yanitskiy
