Skip to content

Commit

Permalink
TC&NE Remove unnecessary TLV_NONE from tests
Browse files Browse the repository at this point in the history
  • Loading branch information
@tuliocasagrande
tuliocasagrande committed Nov 29, 2017
1 parent f3b0c1f commit f93d7b4
Show file tree
Hide file tree
Showing 5 changed files with 17 additions and 17 deletions.
2 changes: 1 addition & 1 deletion src/otrv4.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2274,7 +2274,7 @@ otr4_err_t otrv4_prepare_to_send_message(string_t *to_send,
// Optional. Client might want or not to disguise the length of
// message
if (otr->conversation->client->pad) {
if (append_padding_tlv(tlvs, strlen(message)))
if (append_padding_tlv(&tlvs, strlen(message)))
return OTR4_ERROR;
}

Expand Down
12 changes: 6 additions & 6 deletions src/test/test_api.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ void test_api_interactive_conversation(void) {

// Alice sends a data message
string_t to_send = NULL;
tlv_t *tlv = otrv4_tlv_new(OTRV4_TLV_NONE, 0, NULL);
tlv_t *tlv = NULL;
otr4_err_t err;

for (message_id = 2; message_id < 5; message_id++) {
Expand Down Expand Up @@ -283,7 +283,7 @@ void test_api_non_interactive_conversation(void) {

// Bob sends a data message
string_t to_send = NULL;
tlv_t *tlv = otrv4_tlv_new(OTRV4_TLV_NONE, 0, NULL);
tlv_t *tlv = NULL;
otr4_err_t err;

// TODO: this is usually set up by the querry or whitespace,
Expand Down Expand Up @@ -477,7 +477,7 @@ void test_api_non_interactive_conversation_with_enc_msg(void) {

// Bob sends a data message
string_t to_send = NULL;
tlv_t *tlv = otrv4_tlv_new(OTRV4_TLV_NONE, 0, NULL);
tlv_t *tlv = NULL;
otr4_err_t err;

// TODO: this is usually set up by the querry or whitespace,
Expand Down Expand Up @@ -647,7 +647,7 @@ static void do_ake_otr3(otrv4_t *alice, otrv4_t *bob) {

void test_api_conversation_v3(void) {
OTR4_INIT;
tlv_t *tlv = otrv4_tlv_new(OTRV4_TLV_NONE, 0, NULL);
tlv_t *tlv = NULL;

otr4_client_state_t *alice_state = otr4_client_state_new(NULL);
set_up_client_state(alice_state, ALICE_IDENTITY, PHI, 1);
Expand Down Expand Up @@ -1007,7 +1007,7 @@ void test_api_smp_abort(void) {
void test_api_extra_sym_key(void) {
OTR4_INIT;

tlv_t *tlv = otrv4_tlv_new(OTRV4_TLV_NONE, 0, NULL);
tlv_t *tlv = NULL;

otr4_client_state_t *alice_state = otr4_client_state_new(NULL);
otr4_client_state_t *bob_state = otr4_client_state_new(NULL);
Expand Down Expand Up @@ -1087,7 +1087,7 @@ void test_api_extra_sym_key(void) {

void test_dh_key_rotation(void) {
OTR4_INIT;
tlv_t *tlv = otrv4_tlv_new(OTRV4_TLV_NONE, 0, NULL);
tlv_t *tlv = NULL;
otr4_client_state_t *alice_state = otr4_client_state_new(NULL);
otr4_client_state_t *bob_state = otr4_client_state_new(NULL);

Expand Down
14 changes: 7 additions & 7 deletions src/test/test_tlv.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,19 +65,19 @@ void test_append_padding_tlv() {
uint8_t smp2_data[2] = {0x03, 0x04};

tlv_t *tlv = otrv4_tlv_new(OTRV4_TLV_SMP_MSG_2, sizeof(smp2_data), smp2_data);

otr4_err_t err = append_padding_tlv(tlv, 6);

otr4_err_t err = append_padding_tlv(&tlv, 6);
otrv4_assert(err == OTR4_SUCCESS);
assert_tlv_structure(tlv->next, OTRV4_TLV_PADDING, 245, smp2_data, false);

otrv4_tlv_free(tlv);

tlv = otrv4_tlv_new(OTRV4_TLV_SMP_MSG_2, sizeof(smp2_data), smp2_data);

err = append_padding_tlv(tlv, 500);

err = append_padding_tlv(&tlv, 500);
assert_tlv_structure(tlv->next, OTRV4_TLV_PADDING, 7, smp2_data, false);
otrv4_tlv_free(tlv);

tlv = NULL;
err = append_padding_tlv(&tlv, 500);
otrv4_assert(err == OTR4_SUCCESS);
otrv4_assert(tlv);
otrv4_tlv_free(tlv);
}
4 changes: 2 additions & 2 deletions src/tlv.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ tlv_t *otrv4_padding_tlv_new(size_t len) {
return tlv;
}

otr4_err_t append_padding_tlv(tlv_t *tlvs, int message_len) {
otr4_err_t append_padding_tlv(tlv_t **tlvs, int message_len) {

This comment has been minimized.

Sign in to view

Copy link
@claucece

claucece Nov 29, 2017

Member

Why use the double pointer here? Any particular reason?

This comment has been minimized.

Sign in to view

Copy link
@tuliocasagrande

tuliocasagrande Nov 30, 2017 •
edited

Author Contributor

Hello @claucece, thank you for raising this question.
We decided to send the address of the pointer tlvs, because when it's pointing to NULL before calling append_padding_tlv, we can changed it inside the function. For instance, check this f93d7b4#diff-5e3d9bf1b7212b2d5c72dc2163ca317dR78. At line 78 it's pointing to NULL and at line 81 it's not anymore.

This comment has been minimized.

Sign in to view

Copy link
@claucece

claucece Nov 30, 2017 •
edited

Member

mmm this for me is something that can be fixed by maybe fixing the inside functions rather that changing what is passed to api functions. Take for example what was done:

  1. Pass NULL to any function with padding true, let's say 'otrv4_prepare_to_send_message' as the tlv.
  2. With the appending_padding_tlv, then, you actually take the address of NULL, dereferencing NULL and, therefore, seg faulting or crashing or who knows.

This might be an odd case, but maybe is better to not do it.

Or let's say for example that you don't set the TLV to NULL at the beginning or that you set those TLV to something else, for example, OTRV4_TLV_NONE. The app will get on the undefined behavior section and maybe crash.
This can be a way that someone can modify and make the app crash, maybe.

This comment has been minimized.

Sign in to view

Copy link
@natalieesk

natalieesk Nov 30, 2017

Contributor

Thanks for pointing this out @claucece we're going to have a chat about it.

This comment has been minimized.

Sign in to view

Copy link
@claucece

claucece Nov 30, 2017

Member

with whom?

This comment has been minimized.

Sign in to view

Copy link
@claucece

claucece Nov 30, 2017

Member

oohhh.. I see this was a pairing..ok, my bad ahahahhahahhaha

This comment has been minimized.

Sign in to view

Copy link
@claucece

claucece Dec 7, 2017 •
edited

Member

so, what was the result of the chat? :)

This comment has been minimized.

Sign in to view

Copy link
@natalieesk

natalieesk Jan 9, 2018

Contributor

A TODO has been added to clean up the way the tlvs are being used at the api level.

This comment has been minimized.

Sign in to view

Copy link
@claucece

claucece Jan 9, 2018

Member

thanks!

This comment has been minimized.

Sign in to view

Copy link
@olabini

olabini Apr 10, 2018

Contributor

OK, all this stuff has been simplified and hopefully fixed as part of #48
int padding_granularity = 256;
int header_len = 4;
int nul_byte_len = 1;
Expand All @@ -171,7 +171,7 @@ otr4_err_t append_padding_tlv(tlv_t *tlvs, int message_len) {
if (!padding_tlv)
return OTR4_ERROR;

tlvs = append_tlv(tlvs, padding_tlv);
*tlvs = append_tlv(*tlvs, padding_tlv);

return OTR4_SUCCESS;
}
2 changes: 1 addition & 1 deletion src/tlv.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,6 @@ tlv_t *otrv4_parse_tlvs(const uint8_t *src, size_t len);

tlv_t *append_tlv(tlv_t *tlvs, tlv_t *new_tlv);

otr4_err_t append_padding_tlv(tlv_t *tlvs, int message_len);
otr4_err_t append_padding_tlv(tlv_t **tlvs, int message_len);

#endif

0 comments on commit f93d7b4

Please sign in to comment.