Re: Some questions about the "option block" interface in libwiretap

[Anthony Coddington <Anthony.Coddington () endace com>]

Hi,

Having worked with the wtap_optionblock generic option block system beyond refactoring existing code, I have some 
thoughts on the current design that may be relevant. I have numbered paragraphs from 10 so we don't get confused.

 Michael Mann <mmann78 () netscape net> wrote:
> On May 15, 2016, at 6:40 PM, Guy Harris <guy () alum mit edu> wrote:
> > 3) What mechanisms are available for handling block/record types, or options, not currently supported by pcapng, 
> > but that might be provided by other file types? Hadriel Kaplan suggested getting a Private Enterprise Number (PEN) 
> > for wireshark.org, and using custom blocks and options for this purpose; have we gotten a PEN for wireshark.org 
> > yet?
> The current interface may need another layer of refactoring but the idea was that you would have a block and just 
> call wtap_optionblock_add_option to add your options.  For "defined but not yet used" options of the standard blocks 
> that turn into "now used" options, the wtap_optionblock_add_option would be done within the creation of that 
> standard block.  The creation of "standard blocks" and their option types is where we may need another layer of 
> registration and refactoring. Copying/reading/writing on any/all blocks is supposed to happen within the "option 
> block" functionality (where default behavior would be "inclusive" in copying blocks).  I wasn't sure where "merging" 
> would go and if that would be part of the "option block" functionality (at least for "provided" blocks/options) or 
> left to external tools (like mergecap and editcap).

 10) Generic versus filetype-specific block types: I think the current implementation is too specific to PCAP-NG 
blocks. This makes sense for highly specialized blocks but common metadata that can be (and are) supported across 
formats used by Wireshark like interface information (IDB/ISB), capture information (SHB) and potentially things like 
name resolution blocks, should be as agnostic as possible. The same block type could then be extended with new option 
types for format-specific information. If that is via a Wireshark PEN and custom option that is fine, but it shouldn't 
be too cumbersome to use compared to native PCAP-NG options for simple types. In particular I don't think the mandatory 
struct makes much sense in most cases. Other formats should not be required to provide any options not directly 
required by Wireshark, regardless of whether they are part of a fixed header in PCAP-NG. Examples of this could include 
IDB snap_len and SHB section_length. Ideally Wireshark-wide timestamp formats would also be used or at least included.

 11) Self-writing/serializing block types: For the same reason I really don't think self-writing of PCAP-NG blocks 
belongs in wtap_opttypes.c (as relatively recently added in https://code.wireshark.org/review/#/c/14357/). I think the 
optionblock should act more like a well-known list of generic and filetype specific options that fit into that metadata 
category. Perhaps formats could register a write function each for the block type, but I personally don't see much 
benefit in this. The format might as well treat it similarly to foreign encap types in its dump function, where the 
wiretap determines how the information should be converted or written out for that format.

 12) Multiple options: As Guy Harris has mentioned, it should be possible to add multiple options of a given type such 
as (but not limited to!) multiple comments. Personally I think a list-based approach works well here (possibly also 
indexed by a hash table or something like a GSequence) as it also preserves ordering which may be important for some 
formats. Perhaps an append() vs add() (or set()) distinction could be made, for replacing an existing value rather than 
adding another. On the display side, multiple comments for a packet may even come from multiple blocks.

 13) Checking for and merging options: There seems to be some conflation between adding options to blocks and 
registering "standard" option types to block types. There doesn't appear to be a way to determine whether an option was 
already present in the record or simply has a default value, as all options are 'added' up front. As far as I can tell 
wtap_optionblock_copy_options() will always take the 'existing option' path, even when that option was not explicitly 
set before. This effectively means all options get overridden rather than a merge. Personally I think some kind of 
merge behaviour is useful, as it also allows partial updates of metadata while reading or editing the file. Merging of 
individual options could probably be limited to simple replacing (or removing) the value of an existing option though 
and possibly should be handled by the merger. This is the one place where per-format optionblock types and a merge 
callback could be useful, but personally I'm not sure the loss of other genericness is worth it. A hybrid approach 
might work where a merge callback of the reading wiretap is called and generic options used, possibly combined with 15) 
below. I'm undecided on whether an automatic merge of the union of options together when merging (which I am assuming 
what is meant by Michael Mann by "inclusive" copying) is a good idea. In many cases it probably makes sense, but in 
some cases it may be contradictory in a way that is only determinable by understanding the native format. Similar 
issues come up when deciding whether to merge IDBs, what happens here currently? Perhaps a dedicated flag similar to 
PCAP-NG custom option copy/don't copy behaviour could be useful.

 14) String options: I think the current behaviour of duplicating string options with strdup() does more harm than 
good. As there is no variant that takes a string length, almost every single current string option is copied to a 
temporary variable, copied again by wtap_optionblock_set_string() and then immediately freed by the caller. This even 
applies to options that simply need to be copied directly from a non-null-terminated TLV field out of the record 
header. Formatting is also not supported but this is a less common case. A contract on how memory passed to the 
function is allocated like the old behaviour might make more sense, or at least make wtap_optionblock_set_string() take 
a string length which is good for safety anyway.

 15) Custom options: I think adopting a custom option system where the user can specify a native binary part (e.g. for 
saving the file in the same format after modification) and a string/integer part (or callback to the original wiretap 
to get this) that can be used for display or conversion makes a lot of sense. I thought this was how PCAP-NG worked, 
but it appears to be an either/or thing there.

 16) wtap_optionblock_t being an opaque pointer rather than a struct is rather confusing and inconsistent with most of 
the rest of Wireshark and even within the optionblock code itself. It looks nice when used with g_array but a GPtrArray 
could potentially be used in its place and the naming is confusing. Alternatively the struct could be exposed but 
marked private like most of wtap and epan.

 Guy Harris <guy () alum mit edu> wrote:
> 4) The existence of wtap_file_get_shb() seems to imply that a file has *a* Section Header Block, but a pcapng file 
> could have multiple SHBs; we don't currently support that, but we should be prepared to do so in the future.
>
> A file can also have multiple Name Resolution Blocks as well; as the pcapng specification says:
 ...
> so we should not have routines that assume a single NRB. Perhaps the routines in question should take an array of 
> NRBs - combining the NRBs into a single table would lose information about which names were resolved by which name 
> servers.

 17) Some option blocks, such as interface statistics blocks, name resolution or even interface configuration may also 
only apply to a particular time/frame range. It would be nice to support this generically somehow, but this may require 
more thought.

 Guy Harris <guy () alum mit edu> wrote:
> For example, we currently don't handle packet metadata very cleanly.  We have a bunch of WTAP_ENCAP_ values that 
> correspond to a regular encapsulation plus file-type-specific metadata.
 ...
> In addition, the metadata that doesn't come directly from the packet data should probably be put into a Buffer 
> rather than a union as is done now; that way we don't have a hard upper limit on the amount of metadata (the ERF 
> handler imposes a limit, about which Anthony Coddington of Endace has commented).
>
> And I think there are still some issues with mergecap that would require better handling of IDBs...
>
> ...and we need to think about whether we're correctly handling all of:
>
>       files that have a per-file link-layer header type (most file formats, including pcap);
>
>       files that have per-interface link-layer header types (such as pcapng);
>
>       files that have per-packet link-layer header types.

 18) How this all fits in with REC_TYPE_FT_SPECIFIC_ records needs consideration, currently it is necessary to dissect 
everything twice to also display in the epan tree. One option would be to allow epan to do things like add comments and 
update other metadata while doing a safely bounded parse. I'm not sure how this interacts with simpler tools like 
mergecap though? Do these use libwiretap on its own? On the subject of buffers and unions it is also not possible to 
use a ft_specfic_record_phdr together with another phdr type, and PCAP-NG assumes it is always a PCAP-NG block type 
code. It may also make sense to have filetype specific not-on-wire pseudo header and trailer buffers or more carefully 
distinguishing record versus captured lengths (currently it is not possible to dissect data not accounted for and 
counted as 'captured' bytes), but that is probably a separate discussion.

 I think the general idea makes a lot of sense and can improve the flexibility of Wireshark, and prefer it to the 
previously proposed idea of synthesizing special record types (non-synthesized special record types make sense though). 
I just think the API needs some tweaks to make it more generic.

Thanks,
Anthony Coddington

Software Engineer – CTO Office
Endace

 References:
https://code.wireshark.org/review/#/c/13667/
https://code.wireshark.org/review/#/c/14300/
https://code.wireshark.org/review/#/c/14357/
 Other earlier discussions:
https://wiki.wireshark.org/WiretapPcapng
https://wiki.wireshark.org/Development/PcapngCustom
https://code.wireshark.org/review/#/c/9729/
___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe