Message ID | 20200214172417.11217-1-phil@nwl.cc |
---|---|
State | Not Applicable |
Delegated to: | Pablo Neira |
Headers | show |
Series | [libnftnl] src: Fix nftnl_assert() on data_len | expand |
On Fri, Feb 14, 2020 at 06:24:17PM +0100, Phil Sutter wrote: > Typical idiom for *_get_u*() getters is to call *_get_data() and make > sure data_len matches what each of them is returning. Yet they shouldn't > trust *_get_data() to write into passed pointer to data_len since for > chains and NFTNL_CHAIN_DEVICES attribute, it does not. Make sure these > assert() calls trigger in those cases. The intention to catch for unset attributes through the assertion, right?
On Fri, Feb 14, 2020 at 06:32:47PM +0100, Pablo Neira Ayuso wrote: > On Fri, Feb 14, 2020 at 06:24:17PM +0100, Phil Sutter wrote: > > Typical idiom for *_get_u*() getters is to call *_get_data() and make > > sure data_len matches what each of them is returning. Yet they shouldn't > > trust *_get_data() to write into passed pointer to data_len since for > > chains and NFTNL_CHAIN_DEVICES attribute, it does not. Make sure these > > assert() calls trigger in those cases. > > The intention to catch for unset attributes through the assertion, > right? No, this is about making sure that no wrong getter is called, e.g. nftnl_chain_get_u64() with e.g. NFTNL_CHAIN_HOOKNUM attribute which is only 32bits. Cheers, Phil
On Fri, Feb 14, 2020 at 06:34:50PM +0100, Phil Sutter wrote: > On Fri, Feb 14, 2020 at 06:32:47PM +0100, Pablo Neira Ayuso wrote: > > On Fri, Feb 14, 2020 at 06:24:17PM +0100, Phil Sutter wrote: > > > Typical idiom for *_get_u*() getters is to call *_get_data() and make > > > sure data_len matches what each of them is returning. Yet they shouldn't > > > trust *_get_data() to write into passed pointer to data_len since for > > > chains and NFTNL_CHAIN_DEVICES attribute, it does not. Make sure these > > > assert() calls trigger in those cases. > > > > The intention to catch for unset attributes through the assertion, > > right? > > No, this is about making sure that no wrong getter is called, e.g. > nftnl_chain_get_u64() with e.g. NFTNL_CHAIN_HOOKNUM attribute which is > only 32bits. I think it will also catch the case I'm asking. If attribute is unset, then nftnl_chain_get_data() returns NULL and the assertion checks data_len, which has not been properly initialized.
Hi Pablo, On Fri, Feb 14, 2020 at 06:42:00PM +0100, Pablo Neira Ayuso wrote: > On Fri, Feb 14, 2020 at 06:34:50PM +0100, Phil Sutter wrote: > > On Fri, Feb 14, 2020 at 06:32:47PM +0100, Pablo Neira Ayuso wrote: > > > On Fri, Feb 14, 2020 at 06:24:17PM +0100, Phil Sutter wrote: > > > > Typical idiom for *_get_u*() getters is to call *_get_data() and make > > > > sure data_len matches what each of them is returning. Yet they shouldn't > > > > trust *_get_data() to write into passed pointer to data_len since for > > > > chains and NFTNL_CHAIN_DEVICES attribute, it does not. Make sure these > > > > assert() calls trigger in those cases. > > > > > > The intention to catch for unset attributes through the assertion, > > > right? > > > > No, this is about making sure that no wrong getter is called, e.g. > > nftnl_chain_get_u64() with e.g. NFTNL_CHAIN_HOOKNUM attribute which is > > only 32bits. > > I think it will also catch the case I'm asking. If attribute is unset, > then nftnl_chain_get_data() returns NULL and the assertion checks > data_len, which has not been properly initialized. With nftnl_assert() being (shortened): | #define nftnl_assert(val, attr, expr) \ | ((!val || expr) ? \ | (void)0 : __nftnl_assert_fail(attr, __FILE__, __LINE__)) Check for 'expr' (which is passed as 'data_len == sizeof(<something>)') will only happen if 'val' is not NULL. Callers then return like so: | return val ? *val : 0; This means that if you pass an unset attribute to the getter, it will simply return 0. Cheers, Phil
On Sat, Feb 15, 2020 at 01:43:11AM +0100, Phil Sutter wrote: > Hi Pablo, > > On Fri, Feb 14, 2020 at 06:42:00PM +0100, Pablo Neira Ayuso wrote: > > On Fri, Feb 14, 2020 at 06:34:50PM +0100, Phil Sutter wrote: > > > On Fri, Feb 14, 2020 at 06:32:47PM +0100, Pablo Neira Ayuso wrote: > > > > On Fri, Feb 14, 2020 at 06:24:17PM +0100, Phil Sutter wrote: > > > > > Typical idiom for *_get_u*() getters is to call *_get_data() and make > > > > > sure data_len matches what each of them is returning. Yet they shouldn't > > > > > trust *_get_data() to write into passed pointer to data_len since for > > > > > chains and NFTNL_CHAIN_DEVICES attribute, it does not. Make sure these > > > > > assert() calls trigger in those cases. > > > > > > > > The intention to catch for unset attributes through the assertion, > > > > right? > > > > > > No, this is about making sure that no wrong getter is called, e.g. > > > nftnl_chain_get_u64() with e.g. NFTNL_CHAIN_HOOKNUM attribute which is > > > only 32bits. > > > > I think it will also catch the case I'm asking. If attribute is unset, > > then nftnl_chain_get_data() returns NULL and the assertion checks > > data_len, which has not been properly initialized. > > With nftnl_assert() being (shortened): > > | #define nftnl_assert(val, attr, expr) \ > | ((!val || expr) ? \ > | (void)0 : __nftnl_assert_fail(attr, __FILE__, __LINE__)) > > Check for 'expr' (which is passed as 'data_len == sizeof(<something>)') > will only happen if 'val' is not NULL. Callers then return like so: > > | return val ? *val : 0; > > This means that if you pass an unset attribute to the getter, it will > simply return 0. Thanks for explaining, Phil. If the problem is just NFTNL_CHAIN_DEVICES and NFTNL_FLOWTABLE_DEVICES, probably this is just fine? So zero data-length is reversed for arrays and update nftnl_assert() to skip data_len == 0, ie. > | #define nftnl_assert(val, attr, expr) \ > | ((!val || data_len == 0 || expr) ? \ > | (void)0 : __nftnl_assert_fail(attr, __FILE__, __LINE__))
Hi Pablo, On Sat, Feb 15, 2020 at 02:17:13PM +0100, Pablo Neira Ayuso wrote: > On Sat, Feb 15, 2020 at 01:43:11AM +0100, Phil Sutter wrote: > > Hi Pablo, > > > > On Fri, Feb 14, 2020 at 06:42:00PM +0100, Pablo Neira Ayuso wrote: > > > On Fri, Feb 14, 2020 at 06:34:50PM +0100, Phil Sutter wrote: > > > > On Fri, Feb 14, 2020 at 06:32:47PM +0100, Pablo Neira Ayuso wrote: > > > > > On Fri, Feb 14, 2020 at 06:24:17PM +0100, Phil Sutter wrote: > > > > > > Typical idiom for *_get_u*() getters is to call *_get_data() and make > > > > > > sure data_len matches what each of them is returning. Yet they shouldn't > > > > > > trust *_get_data() to write into passed pointer to data_len since for > > > > > > chains and NFTNL_CHAIN_DEVICES attribute, it does not. Make sure these > > > > > > assert() calls trigger in those cases. > > > > > > > > > > The intention to catch for unset attributes through the assertion, > > > > > right? > > > > > > > > No, this is about making sure that no wrong getter is called, e.g. > > > > nftnl_chain_get_u64() with e.g. NFTNL_CHAIN_HOOKNUM attribute which is > > > > only 32bits. > > > > > > I think it will also catch the case I'm asking. If attribute is unset, > > > then nftnl_chain_get_data() returns NULL and the assertion checks > > > data_len, which has not been properly initialized. > > > > With nftnl_assert() being (shortened): > > > > | #define nftnl_assert(val, attr, expr) \ > > | ((!val || expr) ? \ > > | (void)0 : __nftnl_assert_fail(attr, __FILE__, __LINE__)) > > > > Check for 'expr' (which is passed as 'data_len == sizeof(<something>)') > > will only happen if 'val' is not NULL. Callers then return like so: > > > > | return val ? *val : 0; > > > > This means that if you pass an unset attribute to the getter, it will > > simply return 0. > > Thanks for explaining, Phil. If the problem is just > NFTNL_CHAIN_DEVICES and NFTNL_FLOWTABLE_DEVICES, probably this is just > fine? So zero data-length is reversed for arrays and update > nftnl_assert() to skip data_len == 0, ie. > > > | #define nftnl_assert(val, attr, expr) \ > > | ((!val || data_len == 0 || expr) ? \ > > | (void)0 : __nftnl_assert_fail(attr, __FILE__, __LINE__)) Your proposed patch would allow to call e.g.: | nftnl_chain_get_u32(c, NFTNL_CHAIN_DEVICES) This would return (uint32_t)*(&c->dev_array[0]), I highly doubt we should allow this. Unless I miss something, it is certainly a programming error if someone calls any of the nftnl_chain_get_{u,s}* getters on NFTNL_CHAIN_DEVICES attribute. So aborting with error message in nftnl_assert() is not only OK but actually helpful, no? Cheers, Phil
Hi Phil, On Sat, Feb 15, 2020 at 11:58:55PM +0100, Phil Sutter wrote: > Hi Pablo, > > On Sat, Feb 15, 2020 at 02:17:13PM +0100, Pablo Neira Ayuso wrote: > > On Sat, Feb 15, 2020 at 01:43:11AM +0100, Phil Sutter wrote: > > > Hi Pablo, > > > > > > On Fri, Feb 14, 2020 at 06:42:00PM +0100, Pablo Neira Ayuso wrote: > > > > On Fri, Feb 14, 2020 at 06:34:50PM +0100, Phil Sutter wrote: > > > > > On Fri, Feb 14, 2020 at 06:32:47PM +0100, Pablo Neira Ayuso wrote: > > > > > > On Fri, Feb 14, 2020 at 06:24:17PM +0100, Phil Sutter wrote: > > > > > > > Typical idiom for *_get_u*() getters is to call *_get_data() and make > > > > > > > sure data_len matches what each of them is returning. Yet they shouldn't > > > > > > > trust *_get_data() to write into passed pointer to data_len since for > > > > > > > chains and NFTNL_CHAIN_DEVICES attribute, it does not. Make sure these > > > > > > > assert() calls trigger in those cases. > > > > > > > > > > > > The intention to catch for unset attributes through the assertion, > > > > > > right? > > > > > > > > > > No, this is about making sure that no wrong getter is called, e.g. > > > > > nftnl_chain_get_u64() with e.g. NFTNL_CHAIN_HOOKNUM attribute which is > > > > > only 32bits. > > > > > > > > I think it will also catch the case I'm asking. If attribute is unset, > > > > then nftnl_chain_get_data() returns NULL and the assertion checks > > > > data_len, which has not been properly initialized. > > > > > > With nftnl_assert() being (shortened): > > > > > > | #define nftnl_assert(val, attr, expr) \ > > > | ((!val || expr) ? \ > > > | (void)0 : __nftnl_assert_fail(attr, __FILE__, __LINE__)) > > > > > > Check for 'expr' (which is passed as 'data_len == sizeof(<something>)') > > > will only happen if 'val' is not NULL. Callers then return like so: > > > > > > | return val ? *val : 0; > > > > > > This means that if you pass an unset attribute to the getter, it will > > > simply return 0. > > > > Thanks for explaining, Phil. If the problem is just > > NFTNL_CHAIN_DEVICES and NFTNL_FLOWTABLE_DEVICES, probably this is just > > fine? So zero data-length is reversed for arrays and update > > nftnl_assert() to skip data_len == 0, ie. > > > > > | #define nftnl_assert(val, attr, expr) \ > > > | ((!val || data_len == 0 || expr) ? \ > > > | (void)0 : __nftnl_assert_fail(attr, __FILE__, __LINE__)) > > Your proposed patch would allow to call e.g.: > > | nftnl_chain_get_u32(c, NFTNL_CHAIN_DEVICES) > > This would return (uint32_t)*(&c->dev_array[0]), I highly doubt we > should allow this. Unless I miss something, it is certainly a > programming error if someone calls any of the nftnl_chain_get_{u,s}* > getters on NFTNL_CHAIN_DEVICES attribute. So aborting with error message > in nftnl_assert() is not only OK but actually helpful, no? Indeed, good point. I don't think nftnl_flowtable_set_data() is good for these two device array. I just sent a patch, I forgot to finish the _set_array() and _get_array() helpers for the flowtable, the definition in the header file prooves this. Can we introduce these new interfaces? Then, update nftables to use it. Then, at some point, set *data_len = 0 for these array datatypes. Yes, it's a bit longer term, but better fix this interface. But setting all these data_len to zero when in most cases it is going to be thereafter properly set to the datatype length is... Would this work for you? I know it is not so short term.
Hi Pablo, On Tue, Feb 18, 2020 at 02:42:27PM +0100, Pablo Neira Ayuso wrote: > On Sat, Feb 15, 2020 at 11:58:55PM +0100, Phil Sutter wrote: > > On Sat, Feb 15, 2020 at 02:17:13PM +0100, Pablo Neira Ayuso wrote: > > > On Sat, Feb 15, 2020 at 01:43:11AM +0100, Phil Sutter wrote: > > > > On Fri, Feb 14, 2020 at 06:42:00PM +0100, Pablo Neira Ayuso wrote: > > > > > On Fri, Feb 14, 2020 at 06:34:50PM +0100, Phil Sutter wrote: > > > > > > On Fri, Feb 14, 2020 at 06:32:47PM +0100, Pablo Neira Ayuso wrote: > > > > > > > On Fri, Feb 14, 2020 at 06:24:17PM +0100, Phil Sutter wrote: > > > > > > > > Typical idiom for *_get_u*() getters is to call *_get_data() and make > > > > > > > > sure data_len matches what each of them is returning. Yet they shouldn't > > > > > > > > trust *_get_data() to write into passed pointer to data_len since for > > > > > > > > chains and NFTNL_CHAIN_DEVICES attribute, it does not. Make sure these > > > > > > > > assert() calls trigger in those cases. > > > > > > > > > > > > > > The intention to catch for unset attributes through the assertion, > > > > > > > right? > > > > > > > > > > > > No, this is about making sure that no wrong getter is called, e.g. > > > > > > nftnl_chain_get_u64() with e.g. NFTNL_CHAIN_HOOKNUM attribute which is > > > > > > only 32bits. > > > > > > > > > > I think it will also catch the case I'm asking. If attribute is unset, > > > > > then nftnl_chain_get_data() returns NULL and the assertion checks > > > > > data_len, which has not been properly initialized. > > > > > > > > With nftnl_assert() being (shortened): > > > > > > > > | #define nftnl_assert(val, attr, expr) \ > > > > | ((!val || expr) ? \ > > > > | (void)0 : __nftnl_assert_fail(attr, __FILE__, __LINE__)) > > > > > > > > Check for 'expr' (which is passed as 'data_len == sizeof(<something>)') > > > > will only happen if 'val' is not NULL. Callers then return like so: > > > > > > > > | return val ? *val : 0; > > > > > > > > This means that if you pass an unset attribute to the getter, it will > > > > simply return 0. > > > > > > Thanks for explaining, Phil. If the problem is just > > > NFTNL_CHAIN_DEVICES and NFTNL_FLOWTABLE_DEVICES, probably this is just > > > fine? So zero data-length is reversed for arrays and update > > > nftnl_assert() to skip data_len == 0, ie. > > > > > > > | #define nftnl_assert(val, attr, expr) \ > > > > | ((!val || data_len == 0 || expr) ? \ > > > > | (void)0 : __nftnl_assert_fail(attr, __FILE__, __LINE__)) > > > > Your proposed patch would allow to call e.g.: > > > > | nftnl_chain_get_u32(c, NFTNL_CHAIN_DEVICES) > > > > This would return (uint32_t)*(&c->dev_array[0]), I highly doubt we > > should allow this. Unless I miss something, it is certainly a > > programming error if someone calls any of the nftnl_chain_get_{u,s}* > > getters on NFTNL_CHAIN_DEVICES attribute. So aborting with error message > > in nftnl_assert() is not only OK but actually helpful, no? > > Indeed, good point. > > I don't think nftnl_flowtable_set_data() is good for these two device > array. Well, right now it serves as a backend for all attribute setters, and your patch continues in that tradition. So while it may be a bit "rustic", I'd say it's good enough for its purpose. :) > I just sent a patch, I forgot to finish the _set_array() and > _get_array() helpers for the flowtable, the definition in the header > file prooves this. > > Can we introduce these new interfaces? Then, update nftables to use it. > Then, at some point, set *data_len = 0 for these array datatypes. Yes, > it's a bit longer term, but better fix this interface. But setting all > these data_len to zero when in most cases it is going to be thereafter > properly set to the datatype length is... > > Would this work for you? I know it is not so short term. While I think your patch is the right way to providing a sanitized access to the array attributes, I don't think it's really related to what my original patch was fixing, which is: Right now we are preventing users from passing wrong attribute types to getters by checking the attribute length. This does not work for NFTNL_CHAIN_DEVICES or NFTNL_FLOWTABLE_DEVICES because they don't set data_len. Hence the expression in nftnl_asser() call: | nftnl_assert(val, attr, data_len == sizeof(<something>)); Will lead to comparing with garbage from stack. This may in most cases fail as expected, but there's no guarantee. Your patch allows to use "a better" getter/setter for those problematic attributes, but it doesn't prevent the above from happening. My first approach was to make nftnl_chain_get_data() and nftnl_flowtable_get_data() set: | *data_len = 0; for the problematic attributes, but the value is not really correct - a "more correct" value, e.g.: | *data_len = c->dev_array_len * sizeof(char *); Could lead to a pass in getter sanitizing by accident although e.g. nftnl_chain_get_u64() is completely unfit even if c->dev_array_len was 1. So I decided to go the safe way and initialize data_len variables to zero instead which has the benefit of catching new attributes added later as well. If you don't like the approach of initializing all data_len variables, I would rather suggest to go with setting '*data_len = 0' in _get_data() routines as described above. This has the same effect but it's just a two lines change. What do you think? Cheers, Phil
On Tue, Feb 18, 2020 at 07:18:51PM +0100, Phil Sutter wrote: > Hi Pablo, > > On Tue, Feb 18, 2020 at 02:42:27PM +0100, Pablo Neira Ayuso wrote: > > On Sat, Feb 15, 2020 at 11:58:55PM +0100, Phil Sutter wrote: > > > On Sat, Feb 15, 2020 at 02:17:13PM +0100, Pablo Neira Ayuso wrote: > > > > On Sat, Feb 15, 2020 at 01:43:11AM +0100, Phil Sutter wrote: > > > > > On Fri, Feb 14, 2020 at 06:42:00PM +0100, Pablo Neira Ayuso wrote: > > > > > > On Fri, Feb 14, 2020 at 06:34:50PM +0100, Phil Sutter wrote: > > > > > > > On Fri, Feb 14, 2020 at 06:32:47PM +0100, Pablo Neira Ayuso wrote: > > > > > > > > On Fri, Feb 14, 2020 at 06:24:17PM +0100, Phil Sutter wrote: > > > > > > > > > Typical idiom for *_get_u*() getters is to call *_get_data() and make > > > > > > > > > sure data_len matches what each of them is returning. Yet they shouldn't > > > > > > > > > trust *_get_data() to write into passed pointer to data_len since for > > > > > > > > > chains and NFTNL_CHAIN_DEVICES attribute, it does not. Make sure these > > > > > > > > > assert() calls trigger in those cases. > > > > > > > > > > > > > > > > The intention to catch for unset attributes through the assertion, > > > > > > > > right? > > > > > > > > > > > > > > No, this is about making sure that no wrong getter is called, e.g. > > > > > > > nftnl_chain_get_u64() with e.g. NFTNL_CHAIN_HOOKNUM attribute which is > > > > > > > only 32bits. > > > > > > > > > > > > I think it will also catch the case I'm asking. If attribute is unset, > > > > > > then nftnl_chain_get_data() returns NULL and the assertion checks > > > > > > data_len, which has not been properly initialized. > > > > > > > > > > With nftnl_assert() being (shortened): > > > > > > > > > > | #define nftnl_assert(val, attr, expr) \ > > > > > | ((!val || expr) ? \ > > > > > | (void)0 : __nftnl_assert_fail(attr, __FILE__, __LINE__)) > > > > > > > > > > Check for 'expr' (which is passed as 'data_len == sizeof(<something>)') > > > > > will only happen if 'val' is not NULL. Callers then return like so: > > > > > > > > > > | return val ? *val : 0; > > > > > > > > > > This means that if you pass an unset attribute to the getter, it will > > > > > simply return 0. > > > > > > > > Thanks for explaining, Phil. If the problem is just > > > > NFTNL_CHAIN_DEVICES and NFTNL_FLOWTABLE_DEVICES, probably this is just > > > > fine? So zero data-length is reversed for arrays and update > > > > nftnl_assert() to skip data_len == 0, ie. > > > > > > > > > | #define nftnl_assert(val, attr, expr) \ > > > > > | ((!val || data_len == 0 || expr) ? \ > > > > > | (void)0 : __nftnl_assert_fail(attr, __FILE__, __LINE__)) > > > > > > Your proposed patch would allow to call e.g.: > > > > > > | nftnl_chain_get_u32(c, NFTNL_CHAIN_DEVICES) > > > > > > This would return (uint32_t)*(&c->dev_array[0]), I highly doubt we > > > should allow this. Unless I miss something, it is certainly a > > > programming error if someone calls any of the nftnl_chain_get_{u,s}* > > > getters on NFTNL_CHAIN_DEVICES attribute. So aborting with error message > > > in nftnl_assert() is not only OK but actually helpful, no? > > > > Indeed, good point. > > > > I don't think nftnl_flowtable_set_data() is good for these two device > > array. > > Well, right now it serves as a backend for all attribute setters, and > your patch continues in that tradition. So while it may be a bit > "rustic", I'd say it's good enough for its purpose. :) > > > I just sent a patch, I forgot to finish the _set_array() and > > _get_array() helpers for the flowtable, the definition in the header > > file prooves this. > > > > Can we introduce these new interfaces? Then, update nftables to use it. > > Then, at some point, set *data_len = 0 for these array datatypes. Yes, > > it's a bit longer term, but better fix this interface. But setting all > > these data_len to zero when in most cases it is going to be thereafter > > properly set to the datatype length is... > > > > Would this work for you? I know it is not so short term. > > While I think your patch is the right way to providing a sanitized > access to the array attributes, I don't think it's really related to > what my original patch was fixing, which is: > > Right now we are preventing users from passing wrong attribute types to > getters by checking the attribute length. This does not work for > NFTNL_CHAIN_DEVICES or NFTNL_FLOWTABLE_DEVICES because they don't set > data_len. Hence the expression in nftnl_asser() call: > > | nftnl_assert(val, attr, data_len == sizeof(<something>)); > > Will lead to comparing with garbage from stack. This may in most cases > fail as expected, but there's no guarantee. > > Your patch allows to use "a better" getter/setter for those problematic > attributes, but it doesn't prevent the above from happening. > > My first approach was to make nftnl_chain_get_data() and > nftnl_flowtable_get_data() set: > > | *data_len = 0; > > for the problematic attributes, but the value is not really correct - a > "more correct" value, e.g.: > > | *data_len = c->dev_array_len * sizeof(char *); > > Could lead to a pass in getter sanitizing by accident although e.g. > nftnl_chain_get_u64() is completely unfit even if c->dev_array_len was > 1. > > So I decided to go the safe way and initialize data_len variables to zero > instead which has the benefit of catching new attributes added later as > well. > > If you don't like the approach of initializing all data_len variables, I > would rather suggest to go with setting '*data_len = 0' in _get_data() > routines as described above. This has the same effect but it's just a > two lines change. What do you think? If I apply the patch that I'm attaching, then I use the wrong datatype helper: nftnl_flowtable_get_u32(nlo, NFTNL_FLOWTABLE_DEVICES); And I can see: libnftnl: attribute 6 assertion failed in flowtable.c:274
Hi Pablo, On Tue, Feb 18, 2020 at 10:06:11PM +0100, Pablo Neira Ayuso wrote: [...] > If I apply the patch that I'm attaching, then I use the wrong datatype > helper: > > nftnl_flowtable_get_u32(nlo, NFTNL_FLOWTABLE_DEVICES); > > And I can see: > > libnftnl: attribute 6 assertion failed in flowtable.c:274 Yes, that was what I meant with alternative (simpler) approach. Should I submit this change formally or do you want to do it? Thanks, Phil
On Wed, Feb 19, 2020 at 12:02:39AM +0100, Phil Sutter wrote: > Hi Pablo, > > On Tue, Feb 18, 2020 at 10:06:11PM +0100, Pablo Neira Ayuso wrote: > [...] > > If I apply the patch that I'm attaching, then I use the wrong datatype > > helper: > > > > nftnl_flowtable_get_u32(nlo, NFTNL_FLOWTABLE_DEVICES); > > > > And I can see: > > > > libnftnl: attribute 6 assertion failed in flowtable.c:274 > > Yes, that was what I meant with alternative (simpler) approach. Should I > submit this change formally or do you want to do it? Please submit and push it out, thanks.
diff --git a/src/chain.c b/src/chain.c index b4066e4d4e888..62a9249b57930 100644 --- a/src/chain.c +++ b/src/chain.c @@ -385,7 +385,7 @@ const char *nftnl_chain_get_str(const struct nftnl_chain *c, uint16_t attr) EXPORT_SYMBOL(nftnl_chain_get_u32); uint32_t nftnl_chain_get_u32(const struct nftnl_chain *c, uint16_t attr) { - uint32_t data_len; + uint32_t data_len = 0; const uint32_t *val = nftnl_chain_get_data(c, attr, &data_len); nftnl_assert(val, attr, data_len == sizeof(uint32_t)); @@ -396,7 +396,7 @@ uint32_t nftnl_chain_get_u32(const struct nftnl_chain *c, uint16_t attr) EXPORT_SYMBOL(nftnl_chain_get_s32); int32_t nftnl_chain_get_s32(const struct nftnl_chain *c, uint16_t attr) { - uint32_t data_len; + uint32_t data_len = 0; const int32_t *val = nftnl_chain_get_data(c, attr, &data_len); nftnl_assert(val, attr, data_len == sizeof(int32_t)); @@ -407,7 +407,7 @@ int32_t nftnl_chain_get_s32(const struct nftnl_chain *c, uint16_t attr) EXPORT_SYMBOL(nftnl_chain_get_u64); uint64_t nftnl_chain_get_u64(const struct nftnl_chain *c, uint16_t attr) { - uint32_t data_len; + uint32_t data_len = 0; const uint64_t *val = nftnl_chain_get_data(c, attr, &data_len); nftnl_assert(val, attr, data_len == sizeof(int64_t)); @@ -418,7 +418,7 @@ uint64_t nftnl_chain_get_u64(const struct nftnl_chain *c, uint16_t attr) EXPORT_SYMBOL(nftnl_chain_get_u8); uint8_t nftnl_chain_get_u8(const struct nftnl_chain *c, uint16_t attr) { - uint32_t data_len; + uint32_t data_len = 0; const uint8_t *val = nftnl_chain_get_data(c, attr, &data_len); nftnl_assert(val, attr, data_len == sizeof(int8_t)); diff --git a/src/rule.c b/src/rule.c index 8d7e0681cb42c..ffdbbf8e08140 100644 --- a/src/rule.c +++ b/src/rule.c @@ -249,7 +249,7 @@ const char *nftnl_rule_get_str(const struct nftnl_rule *r, uint16_t attr) EXPORT_SYMBOL(nftnl_rule_get_u32); uint32_t nftnl_rule_get_u32(const struct nftnl_rule *r, uint16_t attr) { - uint32_t data_len; + uint32_t data_len = 0; const uint32_t *val = nftnl_rule_get_data(r, attr, &data_len); nftnl_assert(val, attr, data_len == sizeof(uint32_t)); @@ -260,7 +260,7 @@ uint32_t nftnl_rule_get_u32(const struct nftnl_rule *r, uint16_t attr) EXPORT_SYMBOL(nftnl_rule_get_u64); uint64_t nftnl_rule_get_u64(const struct nftnl_rule *r, uint16_t attr) { - uint32_t data_len; + uint32_t data_len = 0; const uint64_t *val = nftnl_rule_get_data(r, attr, &data_len); nftnl_assert(val, attr, data_len == sizeof(uint64_t)); @@ -271,7 +271,7 @@ uint64_t nftnl_rule_get_u64(const struct nftnl_rule *r, uint16_t attr) EXPORT_SYMBOL(nftnl_rule_get_u8); uint8_t nftnl_rule_get_u8(const struct nftnl_rule *r, uint16_t attr) { - uint32_t data_len; + uint32_t data_len = 0; const uint8_t *val = nftnl_rule_get_data(r, attr, &data_len); nftnl_assert(val, attr, data_len == sizeof(uint8_t)); diff --git a/src/set.c b/src/set.c index 651dcfa56022d..6a6f19bc7fbbf 100644 --- a/src/set.c +++ b/src/set.c @@ -303,7 +303,7 @@ const char *nftnl_set_get_str(const struct nftnl_set *s, uint16_t attr) EXPORT_SYMBOL(nftnl_set_get_u32); uint32_t nftnl_set_get_u32(const struct nftnl_set *s, uint16_t attr) { - uint32_t data_len; + uint32_t data_len = 0; const uint32_t *val = nftnl_set_get_data(s, attr, &data_len); nftnl_assert(val, attr, data_len == sizeof(uint32_t)); @@ -314,7 +314,7 @@ uint32_t nftnl_set_get_u32(const struct nftnl_set *s, uint16_t attr) EXPORT_SYMBOL(nftnl_set_get_u64); uint64_t nftnl_set_get_u64(const struct nftnl_set *s, uint16_t attr) { - uint32_t data_len; + uint32_t data_len = 0; const uint64_t *val = nftnl_set_get_data(s, attr, &data_len); nftnl_assert(val, attr, data_len == sizeof(uint64_t));
Typical idiom for *_get_u*() getters is to call *_get_data() and make sure data_len matches what each of them is returning. Yet they shouldn't trust *_get_data() to write into passed pointer to data_len since for chains and NFTNL_CHAIN_DEVICES attribute, it does not. Make sure these assert() calls trigger in those cases. Signed-off-by: Phil Sutter <phil@nwl.cc> --- src/chain.c | 8 ++++---- src/rule.c | 6 +++--- src/set.c | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-)