Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port to libsoup-3.0 #133

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Port to libsoup-3.0 #133

wants to merge 1 commit into from

Conversation

sgn
Copy link

@sgn sgn commented Mar 4, 2024

No description provided.

@sgn sgn force-pushed the libsoup-3.0 branch 3 times, most recently from b1fe9cb to 2263bc6 Compare March 5, 2024 07:21
Copy link
Member

@lukefromdc lukefromdc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This builds and runs, both the clock/calendar and panel weather applet work fine with it installed. That said at least on my machine I still have a lot of other things depending on the old version, but this is one less.

@lukefromdc lukefromdc requested a review from a team March 6, 2024 19:16
@lukefromdc
Copy link
Member

One possible issue here: we just released 1.28 and probably cannot change external dependencies like this within 1.28 and thus probably cannot do this without bumping version to 1.29

Comment on lines -351 to 360
void request_done (WeatherInfo *info, gboolean ok)
void request_done (WeatherInfo *info, GError *error)
{
if (ok) {
if (error == NULL) {
(void) calc_sun (info);
info->moonValid = info->valid && calc_moon (info);
}
} else if (error->code == G_IO_ERROR_CANCELLED)
return; /* Caused by soup_session_abort */
if (!--info->requests_pending)
info->finish_cb (info, info->cb_data);
}
Copy link
Author

@sgn sgn Mar 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, this PR also fixes a heap-use-after-free in current code base.

Step to reprocedure:

  • Enable weather applets (from mate-applets repository)
  • Switch to Location tab
  • Quickly find and switch multiple Location by Find textbox (e.g. type Paris, London, Ho Chi Minh).
  • It will crash

Analysis:

  • On quick Location find and switch, the request will be sent and freed (with weather_info_free) continously.
  • weather_info_free will call weather_info_abort which, in turn, calls soup_session_abort.
  • soup_session_abort will invoke their callback before returning. However, the document is unclear whether its callback has finished or not at the time of returning. In reality, it's not.
  • Later in weather_info_free, the WeatherInfo structure will be freed.
  • In the racy case, after WeatherInfo freed (in weather_info_free), it could be used, either by request_done or info->network_error in metar.c

Fixes:

  • Fix for libsoup-2.4: I can't think of a reliable way (honestly, I don't care that much about a fix for libsoup-2.4)
  • Fix for libsoup-3.0: Not touching WeatherInfo if the request was cancalled, as indicated by the error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants