Right, I see the implication I didn't intend.
I was pointing out that the mismatch between the CA and RI macros would
likely cause some pretty nasty issues on on the RA version (unless
someone had done the same #ifdef shenanigans).
I agree that there are likely 3 TODOs to be gained from this
conversation:
1- Unify MAX_ADDR_STR_SIZE and MAX_ADDR_STR_SIZE_CA into the same place
(perhaps in a new c_common include file?)
2- Create a separate size variable for the route data.
3- Examine all usages of MAX_ADDR_STR_SIZE(_CA) and ensure that the
correct one is being used (or even better,
sizeof(routeData)/sizeof(addr).
-Erich
On Thu, 2015-11-05 at 18:38 +0000, Light, John J wrote:
> Erich,
>
> The _CA is an artifact of the separation of CA and RI, since there wasn't any
> place to define it once.
>
> The real problem is that MAX_ADDR_STR_SIZE is overloaded. It should only be
> used for the one purpose. The routeData field should have its own macro.
>
> Eliminating the #ifdef is the proper fix, not making OCResource match.
>
> John
>
> -----Original Message-----
> From: Keane, Erich
> Sent: Thursday, November 05, 2015 10:27 AM
> To: Light, John J
> Cc: junghyun.oh at samsung.com; jihun.ha at samsung.com; iotivity-dev at
> lists.iotivity.org
> Subject: Re: [dev] Pre-defined length for the Host Info. is not long enough
>
> AH! I see. Sorry for my mis-interpretation.
>
> That definitely is something that should be audited. I see that
> OCResource.cpp uses MAX_ADDR_STR_SIZE (note the lack of _CA), which won't
> match unless a similar #ifdef is happening too.
>
> What you point out is definitely a pretty nasty gotcha that needs looking
> into closely.
>
> On Thu, 2015-11-05 at 18:24 +0000, Light, John J wrote:
> > Erich,
> >
> > You describe a problem that was fixed. I was referring to something else.
> >
> > OCDevAddr and CAEndpoint_t are both defined thusly:
> >
> > typedef struct
> > {
> > CATransportAdapter_t adapter; // adapter type
> > CATransportFlags_t flags; // transport modifiers
> > uint16_t port; // for IP
> > char addr[MAX_ADDR_STR_SIZE_CA]; // address for
> > all
> > uint32_t interface; // usually zero for default
> > interface
> > #if defined (ROUTING_GATEWAY) || defined (ROUTING_EP)
> > char routeData[MAX_ADDR_STR_SIZE_CA]; /**<
> > GatewayId:ClientId of
> >
> > destination. **/
> > #endif
> > } CAEndpoint_t;
> >
> > Notice that both addr and routeData are defined as MAX_ADDR_STR_SIZE_CA.
> > Since 40 is not long enough for the routeData, the author set
> > MAX_ADDR_STR_SIZE_CA as follows:
> >
> > #ifdef RA_ADAPTER
> > #define MAX_ADDR_STR_SIZE_CA (256)
> > #else
> > #define MAX_ADDR_STR_SIZE_CA (40)
> > #endif
> >
> > So when RA_ADAPTER is defined, the host length will be wrong, possibly
> > causing future problems. Without examining the code, I'm concerned that
> > MAX_ADDR_STR_SIZE_CA is used elsewhere as a general string length.
> >
> > John
> >
> >
> > -----Original Message-----
> > From: Keane, Erich
> > Sent: Thursday, November 05, 2015 9:18 AM
> > To: Light, John J
> > Cc: junghyun.oh at samsung.com; jihun.ha at samsung.com;
> > iotivity-dev at lists.iotivity.org
> > Subject: Re: [dev] Pre-defined length for the Host Info. is not long
> > enough
> >
> > This was actually fixed this one a while ago. I believe there to be no
> > large problem.
> >
> > What ended up happening is we were testing the length of the host address
> > in C++ BEFORE pulling the address apart. I changed it to validate after
> > (right before the copy), and it ended up fixing the problem, since (like
> > you said), the actual values ARE less than 40 bytes long.
> >
> > the "coaps://[fe...]:36238" was NEVER put into a buffer defined by
> > MAX_ADDR_STR_SIZE, it was taken in the std::string 'host' parameter to
> > OCResource's ctor.
> >
> > I believe the cause was at one point we fixed a different issue by making
> > the .host() call return the fully qualified string, rather than just the
> > address. The result was the previous assumption (that the host was a
> > valid, address-only component that could be immediately copied in) was made
> > false.
> >
> > You can see here:
> > https://gerrit.iotivity.org/gerrit/#/c/2481/6/resource/src/OCResource.cpp
> > exactly what I mean.
> >
> > On Thu, 2015-11-05 at 16:00 +0000, Light, John J wrote:
> > > Jay,
> > >
> > >
> > >
> > > Please provide more information about your issue. I originally
> > > wrote the data structures you are talking about, and I am concerned
> > > there might be a larger problem now.
> > >
> > >
> > >
> > > The host address field should only contain the address string itself.
> > > The longest of these is a full IPv6 address:
> > > ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff, which is 39 bytes long, plus
> > > one for a terminating null. What other information are you seeing in
> > > the string?
> > >
> > > You report seeing ?coaps://[fe80::a00:27ff:feaf:f911]:36238?, but
> > > that should never be put in a buffer defined by MAX_ADDR_STR_SIZE.
> > > Can you say where that happened?
> > >
> > > The string MAX_ADDR_STR_SIZE should only be used or the host field
> > > of an OCDevAddr or CAEndpoint_t. I see it is now used for other
> > > fields, and with other sizes than 40.
> > >
> > >
> > >
> > > This misuse will lead to larger problems in future. It is an
> > > addition to the considerable technical debt we have on this project.
> > >
> > >
> > >
> > > John Light
> > >
> > > Intel OTC OIC Development
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > From: iotivity-dev-bounces at lists.iotivity.org
> > > [mailto:iotivity-dev-bounces at lists.iotivity.org] On Behalf Of ???
> > > Sent: Wednesday, November 04, 2015 5:39 PM
> > > To: ???; 'iotivity'
> > > Subject: Re: [dev] Pre-defined length for the Host Info. is not long
> > > enough
> > >
> > >
> > >
> > >
> > > Hi. Jay,
> > >
> > >
> > >
> > > I also encountered the situation you described below, but I found
> > > that this problem has been already resolved in the master branch
> > > with a patchset, https://gerrit.iotivity.org/gerrit/#/c/2481/.
> > > Please look into it.
> > >
> > >
> > >
> > > However, I don't know why this patch had not been merged into
> > > 1.0.0-dev branch. In the 1.0.0-dev branch which is 1.0 release
> > > branch, there is still the above problem. May be someone missed
> > > merging the patchset into the branch.
> > >
> > >
> > >
> > > Thank you.
> > >
> > >
> > >
> > > BR, Jihun Ha
> > >
> > >
> > >
> > > ------- Original Message -------
> > >
> > > Sender : ???<junghyun.oh at samsung.com> S5(??)/??/IoT Lab(S/W?? )/????
> > >
> > > Date : 2015-11-04 17:24 (GMT+09:00)
> > >
> > > Title : [dev] Pre-defined length for the Host Info. is not long
> > > enough
> > >
> > >
> > >
> > > Hi all,
> > >
> > >
> > >
> > > Currently, I?m trying to port the ?Secure? version of the IoTivity
> > > into an embedded device.
> > >
> > > While I was tried to create a resource object of a remote resource
> > > from the Resource Client
> > >
> > > using an API ?constructResourceObject?, I found that the Stack
> > > doesn?t allow the host infor.
> > >
> > > of which length exceeds 40 bytes.
> > >
> > > Actually the target Resource server was supporting IPv6 and using
> > > secure version of IoTivity,
> > >
> > > the length of the host infor exceeds 40 bytes.
> > >
> > > Here is the error log & the actual string of the host infor of the
> > > Resource Server..
> > >
> > > ? Error Log
> > >
> > > terminate called after throwing an instance of 'std::length_error'
> > > what(): host address is too long.
> > >
> > >
> > >
> > > ? Host infor.
> > >
> > > coaps://[fe80::a00:27ff:feaf:f911]:36238
> > >
> > >
> > >
> > > As you can notice the length of the host infor. is 41
> > > bytes.(including ?\0? at the end).
> > >
> > > Since the variables defined for the MAX length of the host infor.
> > > are exists both in the RI layer & CA layer
> > >
> > > & used by lots of locations inside the both layers, I think it will
> > > be better if the contributors of the both layers
> > >
> > > take a look at this and fix if necessary.
> > >
> > > (CA : cacommon.h MAX_ADDR_STR_SIZE_CA 40 , RI : octypes.h
> > > MAX_ADDR_STR_SIZE 40)
> > >
> > >
> > >
> > > I?ve created an issue on Jira
> > > (https://jira.iotivity.org/browse/IOT-824)
> > >
> > > I really will be appreciated if someone resolve this issue. J
> > >
> > >
> > >
> > > Thank you.
> > >
> > > Jay.
> > >
> > >
> > >
> > >
> > >
> > > ?????.?????.
> > >
> > >
> > >
> > > Best Regards,
> > >
> > >
> > >
> > > Jihun Ha (???/???, Ph.D.)
> > >
> > > IoT, IoTivity, OIC| IoT Solution Lab
> > >
> > > Software R&D Center | Samsung Electronics Co., Ltd
> > >
> > > Mobile +82 10 2533 7947
> > >
> > > jihun.ha at samsung.com | jhha85 at gmail.com
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > _______________________________________________
> > > iotivity-dev mailing list
> > > iotivity-dev at lists.iotivity.org
> > > https://lists.iotivity.org/mailman/listinfo/iotivity-dev
> >
>
