From e25562baddd7a675510daea9bb6a6e4ab36b2d68 Mon Sep 17 00:00:00 2001 From: Hadriel Kaplan Date: Mon, 20 Jul 2015 11:09:06 -0400 Subject: [PATCH] Pcapng: clean up Section Header Block handling Change-Id: I8516d0c561ed0b63e49a3594027c9c15bb789258 Reviewed-on: https://code.wireshark.org/review/9726 Reviewed-by: Hadriel Kaplan Petri-Dish: Hadriel Kaplan Tested-by: Petri Dish Buildbot Reviewed-by: Anders Broman --- capinfos.c | 15 ++++---- debian/libwiretap0.symbols | 5 ++- editcap.c | 28 +++++++++------ file.c | 37 ++++++-------------- reordercap.c | 8 ++--- summary.c | 5 ++- tshark.c | 16 ++++----- ui/tap_export_pdu.c | 2 +- wiretap/wtap.c | 48 +++++++++++++++++++------- wiretap/wtap.h | 71 +++++++++++++++++++++++++++++++++----- 10 files changed, 150 insertions(+), 85 deletions(-) diff --git a/capinfos.c b/capinfos.c index 3ca451c0cf..dd20593c5e 100644 --- a/capinfos.c +++ b/capinfos.c @@ -1012,7 +1012,7 @@ process_cap_file(wtap *wth, const char *filename) nstime_t prev_time; gboolean know_order = FALSE; order_t order = IN_ORDER; - wtapng_section_t *shb_inf; + const gchar *shb_comment; gchar *p; @@ -1167,19 +1167,18 @@ process_cap_file(wtap *wth, const char *filename) } cf_info.comment = NULL; - shb_inf = wtap_file_get_shb_info(wth); - if (shb_inf) { + shb_comment = wtap_file_get_shb_comment(wth); + if (shb_comment) { /* opt_comment is always 0-terminated by pcapng_read_section_header_block */ - cf_info.comment = g_strdup(shb_inf->opt_comment); - } - g_free(shb_inf); - if (cf_info.comment) { + cf_info.comment = g_strdup(shb_comment); /* multi-line comments would conflict with the formatting that capinfos uses - we replace linefeeds with spaces */ + we replace carriage-returns/linefeeds with spaces */ p = cf_info.comment; while (*p != '\0') { if (*p == '\n') *p = ' '; + if (*p == '\r') + *p = ' '; p++; } } diff --git a/debian/libwiretap0.symbols b/debian/libwiretap0.symbols index 5bd3483823..9f2ea8840f 100644 --- a/debian/libwiretap0.symbols +++ b/debian/libwiretap0.symbols @@ -50,9 +50,12 @@ libwiretap.so.0 libwiretap0 #MINVER# wtap_fdreopen@Base 1.9.1 wtap_file_encap@Base 1.9.1 wtap_file_get_idb_info@Base 1.9.1 - wtap_file_get_shb_info@Base 1.9.1 wtap_file_get_nrb_for_new_file@Base 1.99.9 + wtap_file_get_shb@Base 1.99.9 + wtap_file_get_shb_comment@Base 1.99.9 + wtap_file_get_shb_for_new_file@Base 1.99.9 wtap_free_nrb@Base 1.99.9 + wtap_free_shb@Base 1.99.9 wtap_file_size@Base 1.9.1 wtap_file_tsprec@Base 1.99.0 wtap_file_type_subtype@Base 1.12.0~rc1 diff --git a/editcap.c b/editcap.c index 5457100d8e..a57d012cb9 100644 --- a/editcap.c +++ b/editcap.c @@ -1285,7 +1285,7 @@ DIAG_ON(cast-qual) wtap_file_type_subtype_string(wtap_file_type_subtype(wth))); } - shb_hdr = wtap_file_get_shb_info(wth); + shb_hdr = wtap_file_get_shb_for_new_file(wth); idb_inf = wtap_file_get_idb_info(wth); nrb_hdr = wtap_file_get_nrb_for_new_file(wth); @@ -1320,7 +1320,7 @@ DIAG_ON(cast-qual) if (read_count == 1) { if (split_packet_count > 0 || secs_per_block > 0) { if (!fileset_extract_prefix_suffix(argv[optind+1], &fprefix, &fsuffix)) - exit(2); + goto error_on_exit; filename = fileset_get_filename_by_pattern(block_cnt++, phdr, fprefix, fsuffix); } else { @@ -1340,7 +1340,7 @@ DIAG_ON(cast-qual) if (pdh == NULL) { fprintf(stderr, "editcap: Can't open or create %s: %s\n", filename, wtap_strerror(err)); - exit(2); + goto error_on_exit; } } /* first packet only handling */ @@ -1365,7 +1365,7 @@ DIAG_ON(cast-qual) if (!wtap_dump_close(pdh, &err)) { fprintf(stderr, "editcap: Error writing to %s: %s\n", filename, wtap_strerror(err)); - exit(2); + goto error_on_exit; } block_start.secs = block_start.secs + secs_per_block; /* reset for next interval */ g_free(filename); @@ -1382,7 +1382,7 @@ DIAG_ON(cast-qual) if (pdh == NULL) { fprintf(stderr, "editcap: Can't open or create %s: %s\n", filename, wtap_strerror(err)); - exit(2); + goto error_on_exit; } } } @@ -1394,7 +1394,7 @@ DIAG_ON(cast-qual) if (!wtap_dump_close(pdh, &err)) { fprintf(stderr, "editcap: Error writing to %s: %s\n", filename, wtap_strerror(err)); - exit(2); + goto error_on_exit; } g_free(filename); @@ -1410,7 +1410,7 @@ DIAG_ON(cast-qual) if (pdh == NULL) { fprintf(stderr, "editcap: Can't open or create %s: %s\n", filename, wtap_strerror(err)); - exit(2); + goto error_on_exit; } } } /* split packet handling */ @@ -1747,7 +1747,7 @@ DIAG_ON(cast-qual) filename, wtap_strerror(err)); break; } - exit(2); + goto error_on_exit; } written_count++; } @@ -1781,7 +1781,7 @@ DIAG_ON(cast-qual) if (pdh == NULL) { fprintf(stderr, "editcap: Can't open or create %s: %s\n", filename, wtap_strerror(err)); - exit(2); + goto error_on_exit; } } @@ -1791,9 +1791,9 @@ DIAG_ON(cast-qual) if (!wtap_dump_close(pdh, &err)) { fprintf(stderr, "editcap: Error writing to %s: %s\n", filename, wtap_strerror(err)); - exit(2); + goto error_on_exit; } - g_free(shb_hdr); + wtap_free_shb(shb_hdr); shb_hdr = NULL; wtap_free_nrb(nrb_hdr); nrb_hdr = NULL; @@ -1817,6 +1817,12 @@ DIAG_ON(cast-qual) } return 0; + +error_on_exit: + wtap_free_shb(shb_hdr); + wtap_free_nrb(nrb_hdr); + g_free(idb_inf); + exit(2); } /* Skip meta-information read from file to return offset of real diff --git a/file.c b/file.c index 449269fbc4..207ed6d4fb 100644 --- a/file.c +++ b/file.c @@ -1306,7 +1306,7 @@ cf_merge_files(char **out_filenamep, int in_file_count, fake_interface_ids = TRUE; /* Create SHB info */ - shb_hdr = wtap_file_get_shb_info(in_files[0].wth); + shb_hdr = wtap_file_get_shb_for_new_file(in_files[0].wth); comment_gstr = g_string_new(""); g_string_append_printf(comment_gstr, "%s \n",shb_hdr->opt_comment); g_string_append_printf(comment_gstr, "File created by merging: \n"); @@ -1315,14 +1315,10 @@ cf_merge_files(char **out_filenamep, int in_file_count, for (i = 0; i < in_file_count; i++) { g_string_append_printf(comment_gstr, "File%d: %s \n",i+1,in_files[i].filename); } - shb_hdr->section_length = -1; - /* options */ /* TODO: handle comments from each file being merged */ + if (shb_hdr->opt_comment) + g_free(shb_hdr->opt_comment); shb_hdr->opt_comment = g_string_free(comment_gstr, FALSE); /* NULL if not available */ - shb_hdr->shb_hardware = NULL; /* NULL if not available, UTF-8 string containing the */ - /* description of the hardware used to create this section. */ - shb_hdr->shb_os = NULL; /* NULL if not available, UTF-8 string containing the name */ - /* of the operating system used to create this section. */ shb_hdr->shb_user_appl = g_strdup("Wireshark"); /* NULL if not available, UTF-8 string containing the name */ /* of the application used to create this section. */ @@ -3921,39 +3917,26 @@ cf_unignore_frame(capture_file *cf, frame_data *frame) const gchar * cf_read_shb_comment(capture_file *cf) { - wtapng_section_t *shb_inf; - const gchar *temp_str; - /* Get info from SHB */ - shb_inf = wtap_file_get_shb_info(cf->wth); - if (shb_inf == NULL) - return NULL; - temp_str = shb_inf->opt_comment; - g_free(shb_inf); - - return temp_str; - + return wtap_file_get_shb_comment(cf->wth); } void cf_update_capture_comment(capture_file *cf, gchar *comment) { - wtapng_section_t *shb_inf; + const gchar *shb_comment; /* Get info from SHB */ - shb_inf = wtap_file_get_shb_info(cf->wth); + shb_comment = wtap_file_get_shb_comment(cf->wth); /* See if the comment has changed or not */ - if (shb_inf && shb_inf->opt_comment) { - if (strcmp(shb_inf->opt_comment, comment) == 0) { + if (shb_comment) { + if (strcmp(shb_comment, comment) == 0) { g_free(comment); - g_free(shb_inf); return; } } - g_free(shb_inf); - /* The comment has changed, let's update it */ wtap_write_shb_comment(cf->wth, comment); /* Mark the file as having unsaved changes */ @@ -4681,7 +4664,7 @@ cf_save_records(capture_file *cf, const char *fname, guint save_format, int encap; /* XXX: what free's this shb_hdr? */ - shb_hdr = wtap_file_get_shb_info(cf->wth); + shb_hdr = wtap_file_get_shb_for_new_file(cf->wth); idb_inf = wtap_file_get_idb_info(cf->wth); nrb_hdr = wtap_file_get_nrb_for_new_file(cf->wth); @@ -4913,7 +4896,7 @@ cf_export_specified_packets(capture_file *cf, const char *fname, and then write it out if it's one of the specified ones. */ /* XXX: what free's this shb_hdr? */ - shb_hdr = wtap_file_get_shb_info(cf->wth); + shb_hdr = wtap_file_get_shb_for_new_file(cf->wth); idb_inf = wtap_file_get_idb_info(cf->wth); nrb_hdr = wtap_file_get_nrb_for_new_file(cf->wth); diff --git a/reordercap.c b/reordercap.c index a5d936cead..53e7f45c20 100644 --- a/reordercap.c +++ b/reordercap.c @@ -274,7 +274,7 @@ DIAG_ON(cast-qual) } DEBUG_PRINT("file_type_subtype is %u\n", wtap_file_type_subtype(wth)); - shb_hdr = wtap_file_get_shb_info(wth); + shb_hdr = wtap_file_get_shb_for_new_file(wth); idb_inf = wtap_file_get_idb_info(wth); nrb_hdr = wtap_file_get_nrb_for_new_file(wth); @@ -287,7 +287,7 @@ DIAG_ON(cast-qual) if (pdh == NULL) { fprintf(stderr, "reordercap: Failed to open output file: (%s) - error %s\n", outfile, wtap_strerror(err)); - g_free(shb_hdr); + wtap_free_shb(shb_hdr); wtap_free_nrb(nrb_hdr); exit(1); } @@ -361,11 +361,11 @@ DIAG_ON(cast-qual) if (!wtap_dump_close(pdh, &err)) { fprintf(stderr, "reordercap: Error closing %s: %s\n", outfile, wtap_strerror(err)); - g_free(shb_hdr); + wtap_free_shb(shb_hdr); wtap_free_nrb(nrb_hdr); exit(1); } - g_free(shb_hdr); + wtap_free_shb(shb_hdr); wtap_free_nrb(nrb_hdr); /* Finally, close infile */ diff --git a/summary.c b/summary.c index 8448896326..a84cc5e384 100644 --- a/summary.c +++ b/summary.c @@ -105,7 +105,7 @@ summary_fill_in(capture_file *cf, summary_tally *st) { frame_data *first_frame, *cur_frame; guint32 framenum; - wtapng_section_t* shb_inf; + const wtapng_section_t* shb_inf; iface_options iface; guint i; wtapng_iface_descriptions_t* idb_info; @@ -156,7 +156,7 @@ summary_fill_in(capture_file *cf, summary_tally *st) st->dfilter = cf->dfilter; /* Get info from SHB */ - shb_inf = wtap_file_get_shb_info(cf->wth); + shb_inf = wtap_file_get_shb(cf->wth); if(shb_inf == NULL){ st->opt_comment = NULL; st->shb_hardware = NULL; @@ -167,7 +167,6 @@ summary_fill_in(capture_file *cf, summary_tally *st) st->shb_hardware = shb_inf->shb_hardware; st->shb_os = shb_inf->shb_os; st->shb_user_appl = shb_inf->shb_user_appl; - g_free(shb_inf); } st->ifaces = g_array_new(FALSE, FALSE, sizeof(iface_options)); diff --git a/tshark.c b/tshark.c index 20cb6694db..cb4b4fa0e7 100644 --- a/tshark.c +++ b/tshark.c @@ -3137,14 +3137,12 @@ load_cap_file(capture_file *cf, char *save_file, int out_file_type, wtapng_section_t *shb_hdr = NULL; wtapng_iface_descriptions_t *idb_inf = NULL; wtapng_name_res_t *nrb_hdr = NULL; - char *appname = NULL; struct wtap_pkthdr phdr; Buffer buf; epan_dissect_t *edt = NULL; wtap_phdr_init(&phdr); - shb_hdr = wtap_file_get_shb_info(cf->wth); idb_inf = wtap_file_get_idb_info(cf->wth); #ifdef PCAP_NG_DEFAULT if (idb_inf->interface_data->len > 1) { @@ -3166,12 +3164,13 @@ load_cap_file(capture_file *cf, char *save_file, int out_file_type, snapshot_length = WTAP_MAX_PACKET_SIZE; } + shb_hdr = wtap_file_get_shb_for_new_file(cf->wth); nrb_hdr = wtap_file_get_nrb_for_new_file(cf->wth); /* If we don't have an application name add Tshark */ if (shb_hdr->shb_user_appl == NULL) { - appname = g_strdup_printf("TShark (Wireshark) %s", get_ws_vcs_version_info()); - shb_hdr->shb_user_appl = appname; + /* this is free'd by wtap_free_shb() later */ + shb_hdr->shb_user_appl = g_strdup_printf("TShark (Wireshark) %s", get_ws_vcs_version_info()); } if (linktype != WTAP_ENCAP_PER_PACKET && @@ -3382,8 +3381,7 @@ load_cap_file(capture_file *cf, char *save_file, int out_file_type, break; } wtap_dump_close(pdh, &err); - g_free(shb_hdr); - g_free(appname); + wtap_free_shb(shb_hdr); wtap_free_nrb(nrb_hdr); exit(2); } @@ -3488,8 +3486,7 @@ load_cap_file(capture_file *cf, char *save_file, int out_file_type, break; } wtap_dump_close(pdh, &err); - g_free(shb_hdr); - g_free(appname); + wtap_free_shb(shb_hdr); wtap_free_nrb(nrb_hdr); exit(2); } @@ -3603,8 +3600,7 @@ out: cf->wth = NULL; g_free(save_file_string); - g_free(shb_hdr); - g_free(appname); + wtap_free_shb(shb_hdr); wtap_free_nrb(nrb_hdr); return err; diff --git a/ui/tap_export_pdu.c b/ui/tap_export_pdu.c index 0055c1290f..f62373289a 100644 --- a/ui/tap_export_pdu.c +++ b/ui/tap_export_pdu.c @@ -119,7 +119,7 @@ exp_pdu_file_open(exp_pdu_t *exp_pdu_tap_data) appname = g_strdup_printf("Wireshark %s", get_ws_vcs_version_info()); - shb_hdr = g_new(wtapng_section_t,1); + shb_hdr = g_new0(wtapng_section_t,1); shb_hdr->section_length = -1; /* options */ shb_hdr->opt_comment = g_strdup_printf("Dump of PDUs from %s", cfile.filename); diff --git a/wiretap/wtap.c b/wiretap/wtap.c index 02ca1780a4..e5a6af7021 100644 --- a/wiretap/wtap.c +++ b/wiretap/wtap.c @@ -167,21 +167,32 @@ wtap_file_tsprec(wtap *wth) return wth->file_tsprec; } -wtapng_section_t * -wtap_file_get_shb_info(wtap *wth) +const gchar * +wtap_file_get_shb_comment(wtap *wth) { - wtapng_section_t *shb_hdr; + return wth ? wth->shb_hdr.opt_comment : NULL; +} - if(wth == NULL) - return NULL; - shb_hdr = g_new(wtapng_section_t,1); - shb_hdr->section_length = wth->shb_hdr.section_length; +const wtapng_section_t * +wtap_file_get_shb(wtap *wth) +{ + return wth ? &(wth->shb_hdr) : NULL; +} + +wtapng_section_t * +wtap_file_get_shb_for_new_file(wtap *wth) +{ + wtapng_section_t *shb_hdr; + + if (wth == NULL) + return NULL; + + shb_hdr = g_new0(wtapng_section_t,1); + + shb_hdr->section_length = -1; /* options */ - shb_hdr->opt_comment = wth->shb_hdr.opt_comment; /* NULL if not available */ - shb_hdr->shb_hardware = wth->shb_hdr.shb_hardware; /* NULL if not available, UTF-8 string containing the description of the hardware used to create this section. */ - shb_hdr->shb_os = wth->shb_hdr.shb_os; /* NULL if not available, UTF-8 string containing the name of the operating system used to create this section. */ - shb_hdr->shb_user_appl = wth->shb_hdr.shb_user_appl; /* NULL if not available, UTF-8 string containing the name of the application used to create this section. */ - + shb_hdr->opt_comment = g_strdup(wth->shb_hdr.opt_comment); + /* the rest of the options remain NULL */ return shb_hdr; } @@ -219,6 +230,19 @@ wtap_write_nrb_comment(wtap *wth, gchar *comment) wth->nrb_hdr->opt_comment = comment; } +void +wtap_free_shb(wtapng_section_t *shb_hdr) +{ + if (shb_hdr == NULL) + return; + + g_free(shb_hdr->opt_comment); + g_free(shb_hdr->shb_hardware); + g_free(shb_hdr->shb_os); + g_free(shb_hdr->shb_user_appl); + g_free(shb_hdr); +} + void wtap_write_shb_comment(wtap *wth, gchar *comment) { diff --git a/wiretap/wtap.h b/wiretap/wtap.h index 0fc273e61e..1f9b4e1e84 100644 --- a/wiretap/wtap.h +++ b/wiretap/wtap.h @@ -1222,6 +1222,9 @@ typedef struct wtapng_section_s { * following section. * Section Length equal -1 (0xFFFFFFFFFFFFFFFF) means * that the size of the section is not specified + * Note: if writing to a new file, this length will + * be invalid if anything changes, such as the other + * members of this struct, or the packets written. */ /* options */ gchar *opt_comment; /**< NULL if not available */ @@ -1232,7 +1235,7 @@ typedef struct wtapng_section_s { gchar *shb_os; /**< NULL if not available, UTF-8 string containing the * name of the operating system used to create this section. */ - gchar *shb_user_appl; /**< NULL if not available, UTF-8 string containing the + gchar *shb_user_appl; /**< NULL if not available, UTF-8 string containing the * name of the application used to create this section. */ } wtapng_section_t; @@ -1659,8 +1662,64 @@ WS_DLL_PUBLIC int wtap_file_encap(wtap *wth); WS_DLL_PUBLIC int wtap_file_tsprec(wtap *wth); + +/** + * @brief Gets existing section header block, not for new file. + * @details Returns the pointer to the existing SHB, without creating a + * new one. This should only be used for accessing info, not + * for creating a new file based on existing SHB info. Use + * wtap_file_get_shb_for_new_file() for that. + * + * @param wth The wiretap session. + * @return The existing section header, which must NOT be g_free'd. + */ WS_DLL_PUBLIC -wtapng_section_t* wtap_file_get_shb_info(wtap *wth); +const wtapng_section_t* wtap_file_get_shb(wtap *wth); + +/** + * @brief Gets new section header block for new file, based on existing info. + * @details Creates a new wtapng_section_t section header block and only + * copies appropriate members of the SHB for a new file. In + * particular, the comment string is copied, and any custom options + * which should be copied are copied. The os, hardware, and + * application strings are *not* copied. + * + * @note Use wtap_free_shb() to free the returned section header. + * + * @param wth The wiretap session. + * @return The new section header, which must be wtap_free_shb'd. + */ +WS_DLL_PUBLIC +wtapng_section_t* wtap_file_get_shb_for_new_file(wtap *wth); + +/** + * Free's a section header block and all of its members. + */ +WS_DLL_PUBLIC +void wtap_free_shb(wtapng_section_t *shb_hdr); + +/** + * @brief Gets the section header comment string. + * @details This gets the pointer, without duplicating the string. + * + * @param wth The wtap session. + * @return The comment string. + */ +WS_DLL_PUBLIC +const gchar* wtap_file_get_shb_comment(wtap *wth); + +/** + * @brief Sets or replaces the section header comment. + * @details The passed-in comment string is set to be the comment + * for the section header block. The passed-in string's + * ownership will be owned by the block, so it should be + * duplicated before passing into this function. + * + * @param wth The wiretap session. + * @param comment The comment string. + */ +WS_DLL_PUBLIC +void wtap_write_shb_comment(wtap *wth, gchar *comment); /** * @brief Gets existing interface descriptions. @@ -1669,7 +1728,7 @@ wtapng_section_t* wtap_file_get_shb_info(wtap *wth); * @note The returned pointer must be g_free'd, but its internal * interface_data must not. * - * @param wth The current wtap. + * @param wth The wiretap session. * @return A new struct of the existing section descriptions, which must be g_free'd. */ WS_DLL_PUBLIC @@ -1682,7 +1741,7 @@ wtapng_iface_descriptions_t *wtap_file_get_idb_info(wtap *wth); * * @note Use wtap_free_nrb() to free the returned pointer. * - * @param wth The current wiretap header. + * @param wth The wiretap session. * @return The new name resolution info, which must be wtap_free_nrb'd. */ WS_DLL_PUBLIC @@ -1718,10 +1777,6 @@ const gchar* wtap_get_nrb_comment(wtap *wth); WS_DLL_PUBLIC void wtap_write_nrb_comment(wtap *wth, gchar *comment); -/*** sets/replaces the section header comment ***/ -WS_DLL_PUBLIC -void wtap_write_shb_comment(wtap *wth, gchar *comment); - /*** close the file descriptors for the current file ***/ WS_DLL_PUBLIC void wtap_fdclose(wtap *wth);