From 801554fb799869690a24cd61b540f1e5f8d2fccb Mon Sep 17 00:00:00 2001 From: John Thacker Date: Sun, 11 Jun 2023 10:18:18 -0400 Subject: [PATCH] Add a capture file state for a pending read When not updating the packet list during a capture, the capture file structure isn't set up, but there is a pending capture. We currently treat that as "finished reading", but that means that other code assumes that all the structures are set up and can crash, and also don't prompt regarding unsaved packets when trying to close Wireshark. Add a state for FILE_READ_PENDING that sometimes should be treated similar to FILE_CLOSED and sometimes should be treated similar to FILE_READ_IN_PROGRESS. This fixes a crash when enabling "update packet list during a capture" while a capture is in progress, as well a crash when applying a filter while a capture is in progress but real time packet list updates are off. Keep track of the number of packets that the capture child has reported that haven't been read yet, so that the capture statistics stay accurate even if the pref is toggled. Also run the main status bar statistics at the end, so that if any packets are processed in cf_finish_tail() they are reported. This also restores status bar statistics for when update packet list during a capture is off, which 461fb517d1f75f607eb3cb670b87754bc24b82ca accidentally disabled. Fix #4035 --- capture/capture_session.h | 1 + capture/capture_sync.c | 1 + cfile.h | 1 + file.c | 10 ++++++-- ui/capture.c | 41 ++++++++++++++++++++++---------- ui/logray/logray_main_window.cpp | 12 ++++++---- ui/qt/capture_file.cpp | 2 +- ui/qt/main_status_bar.cpp | 37 ++++++++++++++++------------ ui/qt/packet_list.cpp | 2 +- ui/qt/time_shift_dialog.cpp | 4 ++-- ui/qt/wireshark_main_window.cpp | 12 ++++++---- 11 files changed, 81 insertions(+), 42 deletions(-) Index: wireshark-3.6.24/capture/capture_session.h =================================================================== --- wireshark-3.6.24.orig/capture/capture_session.h +++ wireshark-3.6.24/capture/capture_session.h @@ -97,6 +97,7 @@ struct _capture_session { #endif gboolean session_will_restart; /**< Set when session will restart */ guint32 count; /**< Total number of frames captured */ + guint32 count_pending; /**< Number of frames captured but not yet read */ capture_options *capture_opts; /**< options for this capture */ capture_file *cf; /**< handle to cfile */ wtap_rec rec; /**< record we're reading packet metadata into */ Index: wireshark-3.6.24/capture/capture_sync.c =================================================================== --- wireshark-3.6.24.orig/capture/capture_sync.c +++ wireshark-3.6.24/capture/capture_sync.c @@ -137,6 +137,7 @@ capture_session_init(capture_session *ca cap_session->group = getgid(); #endif cap_session->count = 0; + cap_session->count_pending = 0; cap_session->session_will_restart = FALSE; cap_session->new_file = new_file; Index: wireshark-3.6.24/cfile.h =================================================================== --- wireshark-3.6.24.orig/cfile.h +++ wireshark-3.6.24/cfile.h @@ -25,6 +25,7 @@ extern "C" { /* Current state of file. */ typedef enum { FILE_CLOSED, /* No file open */ + FILE_READ_PENDING, /* A file to read, but haven't opened it yet */ FILE_READ_IN_PROGRESS, /* Reading a file we've opened */ FILE_READ_ABORTED, /* Read aborted by user */ FILE_READ_DONE /* Read completed */ Index: wireshark-3.6.24/file.c =================================================================== --- wireshark-3.6.24.orig/file.c +++ wireshark-3.6.24/file.c @@ -359,7 +359,7 @@ void cf_close(capture_file *cf) { cf->stop_flag = FALSE; - if (cf->state == FILE_CLOSED) + if (cf->state == FILE_CLOSED || cf->state == FILE_READ_PENDING) return; /* Nothing to do */ /* Die if we're in the middle of reading a file. */ @@ -906,7 +906,9 @@ cf_continue_tail(capture_file *cf, volat void cf_fake_continue_tail(capture_file *cf) { - cf->state = FILE_READ_DONE; + if (cf->state == FILE_CLOSED) { + cf->state = FILE_READ_PENDING; + } } cf_read_status_t @@ -1647,6 +1649,11 @@ rescan_packets(capture_file *cf, const c guint32 frames_count; gboolean queued_rescan_type = RESCAN_NONE; + if (cf->state == FILE_CLOSED || cf->state == FILE_READ_PENDING) { + return; + } + + /* Rescan in progress, clear pending actions. */ cf->redissection_queued = RESCAN_NONE; ws_assert(!cf->read_lock); Index: wireshark-3.6.24/ui/capture.c =================================================================== --- wireshark-3.6.24.orig/ui/capture.c +++ wireshark-3.6.24/ui/capture.c @@ -393,16 +393,14 @@ capture_input_new_file(capture_session * if(capture_opts->save_file != NULL) { /* we start a new capture file, close the old one (if we had one before). */ /* (we can only have an open capture file in real_time_mode!) */ - if( ((capture_file *) cap_session->cf)->state != FILE_CLOSED) { - if(capture_opts->real_time_mode) { - cap_session->session_will_restart = TRUE; - capture_callback_invoke(capture_cb_capture_update_finished, cap_session); - cf_finish_tail((capture_file *)cap_session->cf, - &cap_session->rec, &cap_session->buf, &err); - cf_close((capture_file *)cap_session->cf); - } else { - capture_callback_invoke(capture_cb_capture_fixed_finished, cap_session); - } + if (((capture_file*)cap_session->cf)->state == FILE_READ_PENDING) { + capture_callback_invoke(capture_cb_capture_fixed_finished, cap_session); + } else if (((capture_file*)cap_session->cf)->state != FILE_CLOSED) { + cap_session->session_will_restart = TRUE; + capture_callback_invoke(capture_cb_capture_update_finished, cap_session); + cf_finish_tail((capture_file *)cap_session->cf, + &cap_session->rec, &cap_session->buf, &err); + cf_close((capture_file *)cap_session->cf); } g_free(capture_opts->save_file); is_tempfile = FALSE; @@ -523,7 +521,23 @@ capture_input_new_packets(capture_sessio ws_assert(capture_opts->save_file); if(capture_opts->real_time_mode) { + if (((capture_file*)cap_session->cf)->state == FILE_READ_PENDING) { + /* Attempt to open the capture file and set up to read from it. */ + switch (cf_open((capture_file*)cap_session->cf, capture_opts->save_file, WTAP_TYPE_AUTO, cf_is_tempfile((capture_file*)cap_session->cf), &err)) { + case CF_OK: + break; + case CF_ERROR: + /* Don't unlink (delete) the save file - leave it around, + for debugging purposes. */ + g_free(capture_opts->save_file); + capture_opts->save_file = NULL; + capture_kill_child(cap_session); + } + capture_callback_invoke(capture_cb_capture_update_started, cap_session); + } /* Read from the capture file the number of records the child told us it added. */ + to_read += cap_session->count_pending; + cap_session->count_pending = 0; switch (cf_continue_tail((capture_file *)cap_session->cf, to_read, &cap_session->rec, &cap_session->buf, &err)) { @@ -545,6 +559,7 @@ capture_input_new_packets(capture_sessio } } else { cf_fake_continue_tail((capture_file *)cap_session->cf); + cap_session->count_pending += to_read; capture_callback_invoke(capture_cb_capture_fixed_continue, cap_session); } @@ -686,7 +701,7 @@ capture_input_closed(capture_session *ca /* We started a capture; process what's left of the capture file if we were in "update list of packets in real time" mode, or process all of it if we weren't. */ - if(capture_opts->real_time_mode) { + if(((capture_file*)cap_session->cf)->state == FILE_READ_IN_PROGRESS) { cf_read_status_t status; /* Read what remains of the capture file. */ @@ -736,7 +751,7 @@ capture_input_closed(capture_session *ca main_window_quit(); break; } - } else { + } else if (((capture_file*)cap_session->cf)->state == FILE_READ_PENDING) { /* first of all, we are not doing a capture any more */ capture_callback_invoke(capture_cb_capture_fixed_finished, cap_session); Index: wireshark-3.6.24/ui/qt/capture_file.cpp =================================================================== --- wireshark-3.6.24.orig/ui/qt/capture_file.cpp +++ wireshark-3.6.24/ui/qt/capture_file.cpp @@ -97,7 +97,7 @@ CaptureFile::~CaptureFile() bool CaptureFile::isValid() const { - if (cap_file_ && cap_file_->state != FILE_CLOSED) { // XXX FILE_READ_IN_PROGRESS as well? + if (cap_file_ && cap_file_->state != FILE_CLOSED && cap_file_->state != FILE_READ_PENDING) { // XXX FILE_READ_IN_PROGRESS as well? return true; } return false; Index: wireshark-3.6.24/ui/qt/main_status_bar.cpp =================================================================== --- wireshark-3.6.24.orig/ui/qt/main_status_bar.cpp +++ wireshark-3.6.24/ui/qt/main_status_bar.cpp @@ -372,48 +372,42 @@ void MainStatusBar::showCaptureStatistic #ifdef HAVE_LIBPCAP if (cap_file_) { /* Do we have any packets? */ - if (cs_fixed_ && cs_count_ > 0) { - if (prefs.gui_qt_show_selected_packet && rows.count() == 1) { - packets_str.append(QString(tr("Selected Packet: %1 %2 ")) - .arg(rows.at(0)) - .arg(UTF8_MIDDLE_DOT)); - } - packets_str.append(QString(tr("Packets: %1")) - .arg(cs_count_)); - } else if (cs_count_ > 0) { + if (!cs_fixed_) { + cs_count_ = cap_file_->count; + } + if (cs_count_ > 0) { if (prefs.gui_qt_show_selected_packet && rows.count() == 1) { packets_str.append(QString(tr("Selected Packet: %1 %2 ")) .arg(rows.at(0)) .arg(UTF8_MIDDLE_DOT)); } packets_str.append(QString(tr("Packets: %1 %4 Displayed: %2 (%3%)")) - .arg(cap_file_->count) + .arg(cs_count_) .arg(cap_file_->displayed_count) - .arg((100.0*cap_file_->displayed_count)/cap_file_->count, 0, 'f', 1) + .arg((100.0*cap_file_->displayed_count)/cs_count_, 0, 'f', 1) .arg(UTF8_MIDDLE_DOT)); if (rows.count() > 1) { packets_str.append(QString(tr(" %1 Selected: %2 (%3%)")) .arg(UTF8_MIDDLE_DOT) .arg(rows.count()) - .arg((100.0*rows.count())/cap_file_->count, 0, 'f', 1)); - } + .arg((100.0*rows.count())/cs_count_, 0, 'f', 1)); } if (cap_file_->marked_count > 0) { packets_str.append(QString(tr(" %1 Marked: %2 (%3%)")) .arg(UTF8_MIDDLE_DOT) .arg(cap_file_->marked_count) - .arg((100.0*cap_file_->marked_count)/cap_file_->count, 0, 'f', 1)); + .arg((100.0*cap_file_->marked_count)/cs_count_, 0, 'f', 1)); } if (cap_file_->drops_known) { packets_str.append(QString(tr(" %1 Dropped: %2 (%3%)")) .arg(UTF8_MIDDLE_DOT) .arg(cap_file_->drops) - .arg((100.0*cap_file_->drops)/cap_file_->count, 0, 'f', 1)); + .arg((100.0*cap_file_->drops)/cs_count_, 0, 'f', 1)); } if (cap_file_->ignored_count > 0) { packets_str.append(QString(tr(" %1 Ignored: %2 (%3%)")) .arg(UTF8_MIDDLE_DOT) .arg(cap_file_->ignored_count) - .arg((100.0*cap_file_->ignored_count)/cap_file_->count, 0, 'f', 1)); + .arg((100.0*cap_file_->ignored_count)/cs_count_, 0, 'f', 1)); } if (cap_file_->packet_comment_count > 0) { packets_str.append(QString(tr(" %1 Comments: %2")) @@ -430,6 +424,15 @@ void MainStatusBar::showCaptureStatistic .arg(computed_elapsed%1000, 3, 10, QLatin1Char('0'))); } } + } else if (cs_fixed_ && cs_count_ > 0) { + /* There shouldn't be any rows without a cap_file_ but this is benign */ + if (prefs.gui_qt_show_selected_packet && rows.count() == 1) { + packets_str.append(QString(tr("Selected Packet: %1 %2 ")) + .arg(rows.at(0)) + .arg(UTF8_MIDDLE_DOT)); + } + packets_str.append(QString(tr("Packets: %1")) + .arg(cs_count_)); } #endif // HAVE_LIBPCAP @@ -634,6 +637,9 @@ void MainStatusBar::captureEventHandler( case CaptureEvent::Continued: updateCaptureStatistics(ev.capSession()); break; + case CaptureEvent::Finished: + updateCaptureStatistics(ev.capSession()); + break; default: break; } Index: wireshark-3.6.24/ui/qt/time_shift_dialog.cpp =================================================================== --- wireshark-3.6.24.orig/ui/qt/time_shift_dialog.cpp +++ wireshark-3.6.24/ui/qt/time_shift_dialog.cpp @@ -225,11 +225,11 @@ void TimeShiftDialog::applyTimeShift() { const gchar *err_str = NULL; - if (!cap_file_ || cap_file_->state == FILE_CLOSED) return; + if (!cap_file_ || cap_file_->state == FILE_CLOSED || cap_file_->state == FILE_READ_PENDING) return; syntax_err_.clear(); if (cap_file_->state == FILE_READ_IN_PROGRESS) { - syntax_err_ = tr("Time shifting is not available capturing packets."); + syntax_err_ = tr("Time shifting is not available while capturing packets."); } else if (ts_ui_->shiftAllButton->isChecked()) { err_str = time_shift_all(cap_file_, ts_ui_->shiftAllTimeLineEdit->text().toUtf8().constData());