Wireshark-dev: Re: [Wireshark-dev] signedness of comparison functions in ftype-integer.c
On 1/8/07, Martin Mathieson <martin.r.mathieson@xxxxxxxxxxxxxx> wrote:
On 1/4/07, Martin Mathieson <martin.r.mathieson@xxxxxxxxxxxxxx> wrote:
> On 1/3/07, Guy Harris <guy@xxxxxxxxxxxx> wrote:
> > Martin Mathieson wrote:
> >
> > > For the more general problem, I see 2 possible solutions:
> > > (1) have both signed and values in the union, and use the appropriate
> > > signed or unsigned parts of the union in the comparison functions
> > > (2) leave the union as it is with unsigned members, cast values in all
> > > the signed versions instead of the unsigned versions
> >
> > I vote for solution 1 (and have everything that adds a signed value to
> > the protocol tree set the signed value in the union).
>
> The attached patch tries to implement (1), and seems to work (in the
> limited testing I've done). However, it ended up touching more files
> than I expected (a couple of dissectors even!). There are still 2 or
> 3 places where signed and unsigned FT types are still handled together
> by treating the value as guint32, thus potentially losing -ve values
> of FT_INT32 fields, I've preserved the existing behaviour for now.
>
> I would appreciate if someone could review this patch. It does
> correct a real problem, although I expect signed 32 bit fields with
> negative values are pretty rare in most protocols (I'd guess they're
> more likely to appear as generated fields). I suppose 64-bit
> numerical fields theoretically have the same problem, but is
> presumably even less likely to cause real problems...?
>
> Best regards,
> Martin
>
Hi,
I've done more testing, and fixed some problems, the up-to-date patch
is attached. The downside I can see to this change is that code using
fvalue_get_integer() or get_value_integer() (both now in uinteger and
sinteger versions) has to be careful to select the right one - calling
the wrong one would result in an assertion if it doesn't fit the FT
type of the field. See e.g. gtk/rtp_analysis.c's process_node() -
I've made it always use the unsigned accesser, and it appears to be
safe because all of the fields it currently retrieves are unsigned
values.
I believe that this change should be applied. If no-one objects,
should it be done before or after the next release (not sure how
imminent that might be) ?
Hi,
Now that 0.99.5 has been branched, I've checked this in with rev 20472
(*). Please let me know of any problems, the most likely symptom
would be an assert failing (where it was asserting that the sinteger
or uinteger accessor function pointer was NULL for an fvalue_t),
which, if it happens, should be easy to fix. The alternative, which
was to fix the casts, would have been a hack and I don't think would
have fixed all of the problems this fix does.
Best regards,
Martin
(*) Some unintended changes were also submitted. I did 'svn commit'
from my wireshark folder, and thought that by deleting certain files
from the list below the checkin comment, they wouldn't be committed
(like in perforce)...
