compiler warning: unconstify arg passed to OCTET_STRING_noalloc()

Submitted by Neels Hofmeyr on April 14, 2016, 12:49 p.m.

Details

Message ID 1460638140-20008-1-git-send-email-nhofmeyr@sysmocom.de
State New
Series "compiler warning: unconstify arg passed to OCTET_STRING_noalloc()"
Delegated to: Neels Hofmeyr
Headers show

Commit Message

Neels Hofmeyr April 14, 2016, 12:49 p.m.
This is after the patch to libasn1c, removing str arg constness.
---
 src/ranap_msg_factory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/ranap_msg_factory.c b/src/ranap_msg_factory.c
index bdae92e..461df1a 100644
--- a/src/ranap_msg_factory.c
+++ b/src/ranap_msg_factory.c
@@ -181,7 +181,7 @@  struct msgb *ranap_new_msg_dt(uint8_t sapi, const uint8_t *nas, unsigned int nas
 		ies.sapi = RANAP_SAPI_sapi_0;
 
 	/* Avoid copying + later freeing of OCTET STRING */
-	OCTET_STRING_noalloc(&ies.nas_pdu, nas, nas_len);
+	OCTET_STRING_noalloc(&ies.nas_pdu, (uint8_t*)nas, nas_len);
 
 	/* ies -> dt */
 	rc = ranap_encode_directtransferies(&dt, &ies);

Comments

Holger Freyther April 14, 2016, 1:33 p.m.
> On 14 Apr 2016, at 08:49, Neels Hofmeyr <nhofmeyr@sysmocom.de> wrote:
> 
> This is after the patch to libasn1c, removing str arg constness.
> ---
> src/ranap_msg_factory.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/ranap_msg_factory.c b/src/ranap_msg_factory.c
> index bdae92e..461df1a 100644
> --- a/src/ranap_msg_factory.c
> +++ b/src/ranap_msg_factory.c
> @@ -181,7 +181,7 @@ struct msgb *ranap_new_msg_dt(uint8_t sapi, const uint8_t *nas, unsigned int nas
> 		ies.sapi = RANAP_SAPI_sapi_0;
> 
> 	/* Avoid copying + later freeing of OCTET STRING */
> -	OCTET_STRING_noalloc(&ies.nas_pdu, nas, nas_len);
> +	OCTET_STRING_noalloc(&ies.nas_pdu, (uint8_t*)nas, nas_len);

doesn't seem the right way forward. Does OCTET_STRING_noalloc or its users need/want to modify the buffer?

holger
Neels Hofmeyr April 14, 2016, 2:57 p.m.
On Thu, Apr 14, 2016 at 09:33:37AM -0400, Holger Freyther wrote:
> 
> > On 14 Apr 2016, at 08:49, Neels Hofmeyr <nhofmeyr@sysmocom.de> wrote:
> > 
> > This is after the patch to libasn1c, removing str arg constness.
> > ---
> > src/ranap_msg_factory.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/ranap_msg_factory.c b/src/ranap_msg_factory.c
> > index bdae92e..461df1a 100644
> > --- a/src/ranap_msg_factory.c
> > +++ b/src/ranap_msg_factory.c
> > @@ -181,7 +181,7 @@ struct msgb *ranap_new_msg_dt(uint8_t sapi, const uint8_t *nas, unsigned int nas
> > 		ies.sapi = RANAP_SAPI_sapi_0;
> > 
> > 	/* Avoid copying + later freeing of OCTET STRING */
> > -	OCTET_STRING_noalloc(&ies.nas_pdu, nas, nas_len);
> > +	OCTET_STRING_noalloc(&ies.nas_pdu, (uint8_t*)nas, nas_len);
> 
> doesn't seem the right way forward. Does OCTET_STRING_noalloc or its users need/want to modify the buffer?

So, I want to change this:

static inline void OCTET_STRING_noalloc(OCTET_STRING_t *s, const uint8_t *str, int size)
{
        s->buf = str;
        s->size = size;
}

because OCTET_STRING_t.s is not const, and must not be const, will never be const.
OCTET_STRING_noalloc() has no way to tell whether callers will subsequently
modify the buffer or not. So that function must not secretly unconst:

static inline void OCTET_STRING_noalloc(OCTET_STRING_t *s, const uint8_t *str, int size)
{
        s->buf = (uint8_t*)str;
        s->size = size;
}

^ this is bad.

To get rid of compiler warnings, I want to change to more sensible:

static inline void OCTET_STRING_noalloc(OCTET_STRING_t *s, uint8_t *str, int size)
{
        s->buf = str;
        s->size = size;
}

All callers of OCTET_STRING_noalloc() pass a non-const pointer, except that one
above. I just want to unconst that single caller.

OCTET_STRING_t.buf is simply not const. So if we're not going to do an ugly
unconst like this somewhere, we have to change ranap_new_msg_dt() to take a
non-const uint8_t* nas. I didn't want to change that though, because the
function does rightfully signal outwards that *nas is never changed, because
its use of the unconsted value is contained entirely in that function (to
encode a message) and nas is in fact not modified (given that the encoders work
correctly).

Admitted, I should also add a comment.

~Neels
Harald Welte April 15, 2016, 7:52 p.m.
On Thu, Apr 14, 2016 at 04:57:11PM +0200, Neels Hofmeyr wrote:

> All callers of OCTET_STRING_noalloc() pass a non-const pointer, except
> that one above. I just want to unconst that single caller.

I think the other callers should be fixed, rather than aligning the one
correct part of the code.

As indicated, I believe we shouldn't let bad impleentation or API design
by asn1c contaminate our code.  A pointer to input data into an encoding
function should be const.
Neels Hofmeyr April 15, 2016, 7:52 p.m.
On Thu, Apr 14, 2016 at 04:57:11PM +0200, Neels Hofmeyr wrote:
> So, I want to change this:
> 
> static inline void OCTET_STRING_noalloc(OCTET_STRING_t *s, const uint8_t *str, int size)
> {
>         s->buf = str;
>         s->size = size;
> }
> 
> because OCTET_STRING_t.s is not const, and must not be const, will never be const.

I meant OCTET_STRING_t.buf, of course.

~Neels