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

start using const on internal arguments #665

Merged
merged 1 commit into from
Dec 18, 2023
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
2 changes: 1 addition & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ endif()

if(BUILD_WITH_WARNINGS)
if(CMAKE_C_COMPILER_ID MATCHES "Clang" OR CMAKE_C_COMPILER_ID MATCHES "GNU")
set(WARNINGS -Wall -pedantic -Wextra -Werror)
set(WARNINGS -Wall -pedantic -Wextra -Wcast-qual -Werror)
elseif(MSVC)
set(WARNINGS /W4 /WX)
endif()
Expand Down
4 changes: 3 additions & 1 deletion crypto/cipher/aes_gcm_nss.c
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,9 @@ static srtp_err_status_t srtp_aes_gcm_nss_context_init(void *cv,
return (srtp_err_status_cipher_fail);
}

SECItem key_item = { siBuffer, (unsigned char *)key, c->key_size };
/* explicitly cast away const of key */
SECItem key_item = { siBuffer, (unsigned char *)(uintptr_t)key,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the compiler is whing too much if this is an issue.

c->key_size };
c->key = PK11_ImportSymKey(slot, CKM_AES_GCM, PK11_OriginUnwrap,
CKA_ENCRYPT, &key_item, NULL);
PK11_FreeSlot(slot);
Expand Down
4 changes: 3 additions & 1 deletion crypto/cipher/aes_icm_nss.c
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,9 @@ static srtp_err_status_t srtp_aes_icm_nss_context_init(void *cv,
return srtp_err_status_bad_param;
}

SECItem keyItem = { siBuffer, (unsigned char *)key, c->key_size };
/* explicitly cast away const of key */
SECItem keyItem = { siBuffer, (unsigned char *)(uintptr_t)key,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto here

c->key_size };
c->key = PK11_ImportSymKey(slot, CKM_AES_CTR, PK11_OriginUnwrap,
CKA_ENCRYPT, &keyItem, NULL);
PK11_FreeSlot(slot);
Expand Down
3 changes: 2 additions & 1 deletion crypto/hash/hmac_nss.c
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,8 @@ static srtp_err_status_t srtp_hmac_init(void *statev,
return srtp_err_status_bad_param;
}

SECItem key_item = { siBuffer, (unsigned char *)key, key_len };
/* explicitly cast away const of key */
SECItem key_item = { siBuffer, (unsigned char *)(uintptr_t)key, key_len };
Copy link
Contributor

Choose a reason for hiding this comment

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

And here

sym_key = PK11_ImportSymKey(slot, CKM_SHA_1_HMAC, PK11_OriginUnwrap,
CKA_SIGN, &key_item, NULL);
PK11_FreeSlot(slot);
Expand Down
2 changes: 1 addition & 1 deletion crypto/include/datatypes.h
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ void v128_left_shift(v128_t *x, int shift_index);
* verifying authentication tags.
*/

int srtp_octet_string_is_eq(uint8_t *a, uint8_t *b, int len);
int srtp_octet_string_is_eq(const uint8_t *a, const uint8_t *b, int len);

/*
* A portable way to zero out memory as recommended by
Expand Down
4 changes: 2 additions & 2 deletions crypto/math/datatypes.c
Original file line number Diff line number Diff line change
Expand Up @@ -397,14 +397,14 @@ void bitvector_left_shift(bitvector_t *x, int shift)

#endif /* defined(__SSSE3__) */

int srtp_octet_string_is_eq(uint8_t *a, uint8_t *b, int len)
int srtp_octet_string_is_eq(const uint8_t *a, const uint8_t *b, int len)
{
/*
* We use this somewhat obscure implementation to try to ensure the running
* time only depends on len, even accounting for compiler optimizations.
* The accumulator ends up zero iff the strings are equal.
*/
uint8_t *end = b + len;
const uint8_t *end = b + len;
uint32_t accumulator = 0;

#if defined(__SSE2__)
Expand Down
51 changes: 26 additions & 25 deletions srtp/srtp.c
Original file line number Diff line number Diff line change
Expand Up @@ -79,32 +79,32 @@ srtp_debug_module_t mod_srtp = {
#define uint32s_in_rtcp_header 2
#define octets_in_rtp_extn_hdr 4

static srtp_err_status_t srtp_validate_rtp_header(void *rtp_hdr,
int *pkt_octet_len)
static srtp_err_status_t srtp_validate_rtp_header(const void *rtp_hdr,
int pkt_octet_len)
{
srtp_hdr_t *hdr = (srtp_hdr_t *)rtp_hdr;
const srtp_hdr_t *hdr = (const srtp_hdr_t *)rtp_hdr;
int rtp_header_len;

if (*pkt_octet_len < octets_in_rtp_header)
if (pkt_octet_len < octets_in_rtp_header)
return srtp_err_status_bad_param;

/* Check RTP header length */
rtp_header_len = octets_in_rtp_header + 4 * hdr->cc;
if (hdr->x == 1)
rtp_header_len += octets_in_rtp_extn_hdr;

if (*pkt_octet_len < rtp_header_len)
if (pkt_octet_len < rtp_header_len)
return srtp_err_status_bad_param;

/* Verifing profile length. */
if (hdr->x == 1) {
srtp_hdr_xtnd_t *xtn_hdr =
(srtp_hdr_xtnd_t *)((uint32_t *)hdr + uint32s_in_rtp_header +
hdr->cc);
const srtp_hdr_xtnd_t *xtn_hdr =
(const srtp_hdr_xtnd_t *)((const uint32_t *)hdr +
Copy link
Contributor

Choose a reason for hiding this comment

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

Pre-existing, but the double cast here is also a little surprising that it might be necessary. const additions good.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is only because of the usage of uint32s_in_rtp_header, so needs needs the first cast to do the right pointer arithmetic. I have have another change coming that provides some helper functions for the hdr ext stuff as these cast are ugly and murmurous.

uint32s_in_rtp_header + hdr->cc);
int profile_len = ntohs(xtn_hdr->length);
rtp_header_len += profile_len * 4;
/* profile length counts the number of 32-bit words */
if (*pkt_octet_len < rtp_header_len)
if (pkt_octet_len < rtp_header_len)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow. I'm surprised the compiler did not complain about this one.

Copy link
Member Author

Choose a reason for hiding this comment

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

this was due to api change from int * to int

return srtp_err_status_bad_param;
}
return srtp_err_status_ok;
Expand Down Expand Up @@ -1577,7 +1577,7 @@ static srtp_err_status_t srtp_process_header_encryption(
static void srtp_calc_aead_iv(srtp_session_keys_t *session_keys,
v128_t *iv,
srtp_xtd_seq_num_t *seq,
srtp_hdr_t *hdr)
const srtp_hdr_t *hdr)
{
v128_t in;
v128_t salt;
Expand Down Expand Up @@ -1616,11 +1616,11 @@ static void srtp_calc_aead_iv(srtp_session_keys_t *session_keys,
}

srtp_session_keys_t *srtp_get_session_keys(srtp_stream_ctx_t *stream,
uint8_t *hdr,
const unsigned int *pkt_octet_len,
const uint8_t *hdr,
unsigned int pkt_octet_len,
unsigned int *mki_size)
{
unsigned int base_mki_start_location = *pkt_octet_len;
unsigned int base_mki_start_location = pkt_octet_len;
unsigned int mki_start_location = 0;
unsigned int tag_len = 0;
unsigned int i = 0;
Expand Down Expand Up @@ -1731,7 +1731,7 @@ static srtp_err_status_t srtp_estimate_index(srtp_rdbx_t *rdbx,
return srtp_err_status_ok;
}

static srtp_err_status_t srtp_get_est_pkt_index(srtp_hdr_t *hdr,
static srtp_err_status_t srtp_get_est_pkt_index(const srtp_hdr_t *hdr,
srtp_stream_ctx_t *stream,
srtp_xtd_seq_num_t *est,
int *delta)
Expand Down Expand Up @@ -2169,7 +2169,7 @@ srtp_err_status_t srtp_protect_mki(srtp_ctx_t *ctx,
/* we assume the hdr is 32-bit aligned to start */

/* Verify RTP header */
status = srtp_validate_rtp_header(rtp_hdr, pkt_octet_len);
status = srtp_validate_rtp_header(rtp_hdr, *pkt_octet_len);
if (status)
return status;

Expand Down Expand Up @@ -2499,7 +2499,7 @@ srtp_err_status_t srtp_unprotect_mki(srtp_ctx_t *ctx,
/* we assume the hdr is 32-bit aligned to start */

/* Verify RTP header */
status = srtp_validate_rtp_header(srtp_hdr, pkt_octet_len);
status = srtp_validate_rtp_header(srtp_hdr, *pkt_octet_len);
if (status)
return status;

Expand Down Expand Up @@ -2568,9 +2568,9 @@ srtp_err_status_t srtp_unprotect_mki(srtp_ctx_t *ctx,

/* Determine if MKI is being used and what session keys should be used */
if (use_mki) {
session_keys = srtp_get_session_keys(
stream, (uint8_t *)hdr, (const unsigned int *)pkt_octet_len,
&mki_size);
session_keys =
srtp_get_session_keys(stream, (const uint8_t *)hdr,
(unsigned int)*pkt_octet_len, &mki_size);
Copy link
Contributor

Choose a reason for hiding this comment

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

While this cast might be necessary, why do we have a mix of signed and unsigned integer use? That might be a good thing to harmonize. I personally prefer unsigned integers for length values.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is something we have been looking at and yes we need to harmonize on this and other types, it is coming :)
For length I would even go as far as using size_t every where including in the public api in v3!


if (session_keys == NULL)
return srtp_err_status_bad_mki;
Expand Down Expand Up @@ -3574,7 +3574,7 @@ static srtp_err_status_t srtp_calc_aead_iv_srtcp(
srtp_session_keys_t *session_keys,
v128_t *iv,
uint32_t seq_num,
srtcp_hdr_t *hdr)
const srtcp_hdr_t *hdr)
{
v128_t in;
v128_t salt;
Expand Down Expand Up @@ -3873,8 +3873,9 @@ static srtp_err_status_t srtp_unprotect_rtcp_aead(
* If payload encryption is enabled, then the AAD consist of
* the RTCP header and the seq# at the end of the packet
*/
status = srtp_cipher_set_aad(session_keys->rtcp_cipher, (uint8_t *)hdr,
octets_in_rtcp_header);
status =
srtp_cipher_set_aad(session_keys->rtcp_cipher, (const uint8_t *)hdr,
octets_in_rtcp_header);
if (status) {
return (srtp_err_status_cipher_fail);
}
Expand Down Expand Up @@ -4282,9 +4283,9 @@ srtp_err_status_t srtp_unprotect_rtcp_mki(srtp_t ctx,
* Determine if MKI is being used and what session keys should be used
*/
if (use_mki) {
session_keys = srtp_get_session_keys(
stream, (uint8_t *)hdr, (const unsigned int *)pkt_octet_len,
&mki_size);
session_keys =
srtp_get_session_keys(stream, (const uint8_t *)hdr,
(unsigned int)*pkt_octet_len, &mki_size);

if (session_keys == NULL)
return srtp_err_status_bad_mki;
Expand Down
Loading