Re: [Wireshark-dev] FT_STRING, FT_STRINGZPAD, and null padding

[Guy Harris]

On Sep 4, 2020, at 10:56 PM, John Thacker  wrote:

> I don't understand this change, which makes me wonder if I understand 
> FT_STRINGZPAD vs FT_STRING in general:

OK, here's the types of strings that there can be in protocols.

1. Counted strings.  A counted string has a count of the number of octets (it 
might be half the number of octets if it's UTF-16 or UCS-2, but it's really a 
count of octets).  There is nothing special about null characters, other than 
"if some software treats the string as a C string, it won't work correctly if 
the string happens to include a null character, so maybe that software 
shouldn't treat it as a C string".

2. Null-terminated, non-counted strings.  A null-terminated, non-counted, 
string has no count of the number of octets; it runs until a null terminator 
(8-bit, 16-bit, whatever) is seen.  Obviously, that string can't contain a null 
terminator.

3. Padded strings in a fixed-size buffer.  If the string is the length of the 
buffer, there is no null terminator; otherwise, there's a null terminator; the 
protocol may specify that all octets after the null terminator, if the string 
isn't the length of the buffer - 1, must be null, or may say there is no such 
requirement.

4. Strings that are counted *and* null-terminated.  This is redundant, but 
maybe somebody wants to make sure that there's a null at the end, to make life 
easier for implementations that use C strings internally.  This doesn't 
*really* make things easier, unless you're in a 100% controlled environment, 
because, in any environment in which the software has to deal with packets from 
buggy or malicious senders, and uses C strings internally, they have to make 
sure there's a null terminator *anyway* - they *can't* just copy the string and 
blithely assume that a terminating null is part of the counted blob.

For counted strings, it was deemed useful to *warn* if there's a null in the 
middle of the string, as it might indicate a string with which some 
implementations might have a problem.  The commit that added that check was

commit 5c36f6166c30b586be3e6cc600f58e1eb5830eb7
Author: Stig Bjørlykke 
Date:   Mon Aug 27 20:55:36 2018 +0200

epan: Detect trailing stray characters in strings

Trailing stray characters will not show up in the packet tree item
when the string is correctly null terminated. This expert info
will indicate when this occurs, typically from wrongly implemented
protocol encoders.

This will warn about cases like:

  tvb = "foo\0bar"
  proto_tree_add_item(..., tvb, 0, 7, ...)

In the case of a counted string, if the string is a 7-octet value of 
"foo\0bar", it should show up as exactly that in the packet tree item; if not, 
we should fix it to do so.

For null-terminated, non-counted strings, there's no need to check whether 
there's a null - if there isn't, we'll run past the end of the packet, throw an 
exception, and correctly report the packet as malformed.

For padded strings in a fixed-size buffer, about the only check worth making 
is, *if* there's a null character in the string, whether there's anything 
non-null after the null character.  However, that check should *not* be made 
unless the protocol requires it; there are a lot of cases where it does *not* 
require it, and valid packets don't have it.

For strings that are counted and null-terminated, we should obviously make sure 
the *last* character is a null, and report an error if it's not.

Currently, we use FT_STRING for the first of those, FT_STRINGZ for the second 
of those, FT_STRINGZPAD for the third of those, and FT_STRINGZ for the fourth 
of those; the difference between the two FT_STRINGZ cases is whether a length 
was specified or not.

> Everything in proto.c and tvbuff.c treats FT_STRING and FT_STRINGZPAD exactly 
> the same currently, except for checking stray characters. If the length is 
> given as -1, the length to the end of the tvbuff is used. Otherwise, the 
> specified length is checked and copied from the tvbuff, and a NUL is added at 
> the end regardless. The last byte ensures that they both work even if the 
> buffer is not null terminated.
> 
> However, if the string is null terminated somewhere in the middle, all the 
> bytes after that (presumably padding) are still copied into the return value, 
> along with the extra '\0'. This doesn't really cause any problems other than 
> wasting a little bit of memory and time, since the first null ends the string.
> 
> Before that commit, there was absolutely no difference that I could tell. Now 
> the difference is that if the string is terminated by a NUL somewhere in the 
> middle before the specified field length, whether or not the bytes in the 
> padding after that must be null padding. It is FT_STRING that requires that 
> any padding be zero-padding

That's not padding.  That's characters that might be missed 

[Wireshark-dev] FT_STRING, FT_STRINGZPAD, and null padding

[John Thacker]

I don't understand this change, which makes me wonder if I understand
FT_STRINGZPAD vs FT_STRING in general:
https://gitlab.com/wireshark/wireshark/-/commit/a42286524a0e23f5944e988e672a07a5fb395c69


Everything in proto.c and tvbuff.c treats FT_STRING and FT_STRINGZPAD
exactly the same currently, except for checking stray characters. If the
length is given as -1, the length to the end of the tvbuff is used.
Otherwise, the specified length is checked and copied from the tvbuff, and
a NUL is added at the end regardless. The last byte ensures that they both
work even if the buffer is not null terminated.

However, if the string is null terminated somewhere in the middle, all the
bytes after that (presumably padding) are still copied into the return
value, along with the extra '\0'. This doesn't really cause any problems
other than wasting a little bit of memory and time, since the first null
ends the string.

Before that commit, there was absolutely no difference that I could tell.
Now the difference is that if the string is terminated by a NUL somewhere
in the middle before the specified field length, whether or not the bytes
in the padding after that must be null padding. It is FT_STRING that
requires that any padding be zero-padding (and adds an expert warning if
not), whereas FT_STRINGZPAD does not.

While I agree with the commit message that "there are protocols where a
string that's not the full length of the part of the packet for the string
has a null terminator but isn't guaranteed to be fully padded with nulls,"
and that there can be "a separate type for fields where we really *should*
check that the padding is all nulls," I don't see why FT_STRINGZPAD isn't
that latter type. To me, the name FT_STRINGZPAD implies that it is the one
where we really should check that the padding is zero padding.

I would reverse the handling in that commit, and have FT_STRINGZPAD be the
type that does check, and make FT_STRING be the type that does not check
for trailing characters.

The documentation in README.dissector does not really explain the
differences between the use cases, as it seems like FT_STRING can be used
for everything where FT_STRINGZPAD can:

FT_STRING   A string of characters, not necessarily
NULL-terminated, but possibly NULL-padded.This, and
the other string-of-characterstypes, are to be used
for text strings,not raw binary data.
...FT_STRINGZPAD   A NULL-padded string of characters.
   The length is given in the proto_tree_add_item()
call, but may be larger than the length ofthe string,
with extra bytes being NULL padding.This is typically
used for fixed-length fieldsthat contain a string
value that might be shorterthan the fixed length.

John Thacker
___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Managing Gitlab Issues

[Uli Heilmeier]

> Am 04.09.2020 um 14:59 schrieb Dario Lombardo :
> 
> On Fri, Sep 4, 2020 at 1:12 PM Uli Heilmeier  > wrote:
> Hi list,
> 
> I’ve tried to update the instructions to report an issue (fka bug) in the 
> wiki [1].
> 
> There are some things we need to sort out. (Maybe this has already been done 
> on the core list.)
> 
> * Do we want to have labels to mark the status of an issue? With Bugzilla we 
> had Confirmed, Incomplete, In Progress etc. I would like to have labels for 
> status additionally to the existing labels.
> 
> Personally I don't like this. Labels are best to mark the issues as belonging 
> to a component, a version and so on. I don't see the need to stick with 
> gerrit's model, unless we really like it. Again, personally I liked gerrit's 
> labels, but I won't miss it, and I think that model can be left behind.
>  

I'm sorry I was unclear. I’m talking about the Status field of Bugzilla. Not 
Gerrit labels. With Bugzilla a new bug had (normally) the status ‚Unconfirmed‘ 
at the beginning. When someone was able to reproduce the issue the bug had the 
status ‚Confirmed‘. If something was missing (a capture file for example) the 
status was ‚Incomplete‘. When someone was working on a fix the status was ‚In 
Progress‘.
This is currently missing with Gitlab Issues. There is only ‚Open‘ and ‚Closed‘.

Therefore I suggest we use labels to mark the status of an issue.

> 
> * Who should be able to edit issues (e.g. adding labels)? According to Gitlab 
> documentation [2] the Reporter role can do it. However the Reporter role also 
> allows to see issues which are marked as confidentially.
> 
> I'm not sure I got your point. Just a few people (not including the whole 
> core-dev group) have internal access to the project. Just Gerald and a few 
> have. Other code-devs are in a group allowed to merge, but they're not 
> project members. How would you leverage the confidentiality feature?

With Bugzilla I (as a normal contributor) was able to set the status of a bug 
(not limited to my own created ones). Also I was able to correct the 
classification etc. At the moment I’m not able to do this with Gitlab issues. 
When we can have a new group to edit/label issues I’m totally fine. 

>  
> 
> * It would make sense to have templates for issues [3]. Has anyone already 
> prepared this? Otherwise I will create one and submit a MR?
> 
> This would help for sure. Please submit a MR for that.

Will do so.

___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Managing Gitlab Issues

[Dario Lombardo]

On Fri, Sep 4, 2020 at 1:12 PM Uli Heilmeier  wrote:

> Hi list,
>
> I’ve tried to update the instructions to report an issue (fka bug) in the
> wiki [1].
>
> There are some things we need to sort out. (Maybe this has already been
> done on the core list.)
>
> * Do we want to have labels to mark the status of an issue? With Bugzilla
> we had Confirmed, Incomplete, In Progress etc. I would like to have labels
> for status additionally to the existing labels.
>

Personally I don't like this. Labels are best to mark the issues as
belonging to a component, a version and so on. I don't see the need to
stick with gerrit's model, unless we really like it. Again, personally I
liked gerrit's labels, but I won't miss it, and I think that model can be
left behind.


>
> * Who should be able to edit issues (e.g. adding labels)? According to
> Gitlab documentation [2] the Reporter role can do it. However the Reporter
> role also allows to see issues which are marked as confidentially.
>

I'm not sure I got your point. Just a few people (not including the whole
core-dev group) have internal access to the project. Just Gerald and a few
have. Other code-devs are in a group allowed to merge, but they're not
project members. How would you leverage the confidentiality feature?


>
> * It would make sense to have templates for issues [3]. Has anyone already
> prepared this? Otherwise I will create one and submit a MR?
>

This would help for sure. Please submit a MR for that.
___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

[Wireshark-dev] Managing Gitlab Issues

[Uli Heilmeier]

Hi list,

I’ve tried to update the instructions to report an issue (fka bug) in the wiki 
[1].

There are some things we need to sort out. (Maybe this has already been done on 
the core list.)

* Do we want to have labels to mark the status of an issue? With Bugzilla we 
had Confirmed, Incomplete, In Progress etc. I would like to have labels for 
status additionally to the existing labels.

* Who should be able to edit issues (e.g. adding labels)? According to Gitlab 
documentation [2] the Reporter role can do it. However the Reporter role also 
allows to see issues which are marked as confidentially.

* It would make sense to have templates for issues [3]. Has anyone already 
prepared this? Otherwise I will create one and submit a MR?

Cheers

[1] https://gitlab.com/wireshark/wireshark/-/wikis/ReportingBugs
[2] https://docs.gitlab.com/ee/user/permissions.html
[3] https://docs.gitlab.com/ee/user/project/description_templates.html
___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Winflexbison update in choco

[Dario Lombardo]

Other users are reporting an issue in the new version of the choco package.
Read the discussion on

https://chocolatey.org/packages/winflexbison3

I will try to downgrade the package version, until the package gets fixed.
It looks like the new version basically doesn't install. This isn't spotted
unless you install from scratch (that's what CIs do).

On Wed, Sep 2, 2020 at 3:00 PM Dario Lombardo  wrote:

> The build is actually done by the github actions builder. A brand new dir
> gets created every time.
>
>
> https://github.com/crondaemon/wireshark/actions?query=workflow%3A%22Build+Windows%22+branch%3Amaster
>
> On Wed, Sep 2, 2020 at 2:53 PM Graham Bloice 
> wrote:
>
>>
>> On Wed, 2 Sep 2020 at 13:43, Dario Lombardo  wrote:
>>
>>> Hi
>>> Some days ago (30th aug), the choco package winflexbison3 was updated.
>>> Since then, my cmake can't find the LEX_EXECUTABLE as well as the
>>> YACC_EXECUTABLE.
>>> Any idea on what happened?
>>> I've fixed the builds by setting LEX_ and YACC_EXECUTABLE in cmake, but
>>> I don't know whether this is the correct fix or not.
>>> Dario.
>>>
>>>
>> Either delete CMakeCache.txt (to discover all required libs etc. again),
>> or edit it to remove the invalid path references and then CMake will search
>> and find them again.
>>
>> --
>> Graham Bloice
>>
>> ___
>> Sent via:Wireshark-dev mailing list 
>> Archives:https://www.wireshark.org/lists/wireshark-dev
>> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
>>  mailto:wireshark-dev-requ...@wireshark.org
>> ?subject=unsubscribe
>
>
>
> --
>
> Naima is online.
>
>

-- 

Naima is online.
___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe