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.

Sign up for GitHub

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

Price guardian #1716

Open
wants to merge 2 commits into
base: stable
Choose a base branch
from
Open

Price guardian #1716

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df1ad99
[engine.scm] add gnc:price-ref
christopherlam Jul 20, 2023
4e3ddca
[portfolios] don't ref/unref manually, let price-guardian unref
christopherlam Jul 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions bindings/guile/engine.scm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,20 @@
(load-and-reexport (sw_engine)
(gnucash engine business-core)
(gnucash engine gnc-numeric))


(export gnc:price-ref)

(define price-guardian (make-guardian))

(define (reclaim-prices)
(let ((price (price-guardian)))
(when price
(gnc-price-unref price)
(reclaim-prices))))

(add-hook! after-gc-hook reclaim-prices)

(define (gnc:price-ref price)
(gnc-price-ref price)
(price-guardian price))
4 changes: 4 additions & 0 deletions gnucash/report/gnc-report.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,10 @@ gnc_run_report_with_error_handling (gint report_id, gchar ** data, gchar **errms
html = scm_car (res);
captured_error = scm_cadr (res);

/* Force a gc run after the report is finished to ensure that
guardians are called to unref objects */
scm_gc();

if (!scm_is_false (html))
{
*data = gnc_scm_to_utf8_string (html);
Expand Down
11 changes: 2 additions & 9 deletions gnucash/report/reports/standard/advanced-portfolio.scm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ by preventing negative stock balances.<br/>")
(if (gnc-commodity-equiv currency (gnc-price-get-commodity p))
(set! price (gnc-price-invert p))))
price-list)
(gnc-price-ref price)
(gnc:price-ref price)
(gnc-price-list-destroy price-list)
price)))

Expand Down Expand Up @@ -470,7 +470,6 @@ by preventing negative stock balances.<br/>")
(if (not (gnc-numeric-zero-p (gnc:gnc-monetary-amount trans-price)))
;; We can exchange the price from this transaction into the report currency
(begin
(if price (gnc-price-unref price))
(set! pricing-txn trans)
(set! price trans-price)
Comment on lines -473 to 474
Copy link
Member

Choose a reason for hiding this comment

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

This bugs me. Does (gnc:price-ref price) put price or *price (in C's meaning, Guile having no way to express it) on the guardian's list? If it's *price then no problem; otherwise this will leak the original *price and if trans-price will get an extra unref.

Copy link
Contributor Author

@christopherlam christopherlam Jul 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

well, the reports shouldn't generally call gnc-price-ref/unref, and shuould leave it to the guardians. I don't know whether it calls price or *price.

Copy link
Member

Choose a reason for hiding this comment

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

They don't generally, only the two portfolio reports do. Nothing in Scheme except test-engine-extras.scm writes to price objects, so one option would be to copy the few things that reports need from prices (currency, commodity, time, and value) and not swig any of the price API.

Do you understand the problem with (set! price trans-price) if the guardian is using the addresses of price and trans-price instead of the addresses that they point to (e.g. *price)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you understand the problem with (set! price trans-price) if the guardian is using the addresses of price and trans-price instead of the addresses that they point to (e.g. *price)?

Not quite.

Copy link
Member

Choose a reason for hiding this comment

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

There are two GncPrice pointers, p1 and p2. price is initialized to p1 and trans-price to p2. price-guardian is called on price and trans-price when each is initialized, then (set! price trans-price) happens and both price and trans-price equal p2.

If (price-guardian price) causes the guardian to keep track of p1 (what I called the '*price' scenario) then after the set! call it's garbage-collected and returned and unreffed by reclaim-prices. Later when both price and trans-price go out of scope (maybe at the end of the report render)p2 is garbage collected and can be unreffed by reclaim-prices. All good.

But if (price-guardian price) keeps a reference to price (the 'price' scenario) then it's not guarding p1. It doesn't return anything until price or trans-price go out of scope, and when they do it returns p2 for each one, so p1 leaks and p2 gets unreffed twice. Not good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see slightly and don't know the answer.

Copy link
Member

Choose a reason for hiding this comment

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

Then you need to find out. If it's the latter case then the set! has to go, it's a potential use-after-free crasher.

Alternative: Remove all of the price refs and unrefs, no guardians. I don't think any of these calculations will be interrupted by a status bar update, and even if they can the user would have to somehow queue a price-delete to run during an idle for a price to become invalid while the report is loading. Pretty darn unlikely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternative: Remove all of the price refs and unrefs, no guardians. I don't think any of these calculations will be interrupted by a status bar update, and even if they can the user would have to somehow queue a price-delete to run during an idle for a price to become invalid while the report is loading. Pretty darn unlikely.

So, #1403 resurrected.

Copy link
Member

Choose a reason for hiding this comment

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

OK, probably safer than this. But copying it into a scheme record would be safer still.

(gnc:debug "Transaction price is " (gnc:monetary->string price))
Expand Down Expand Up @@ -942,15 +941,9 @@ by preventing negative stock balances.<br/>")
row-style
activecols)

(if (and (not use-txn) price) (gnc-price-unref price))
(table-add-stock-rows-internal rest (not odd-row?))
)
(begin
(if (and (not use-txn) price) (gnc-price-unref price))
(table-add-stock-rows-internal rest odd-row?)
)
)
)))
(table-add-stock-rows-internal rest odd-row?)))))

(set! work-to-do (gnc:accounts-count-splits accounts))
(table-add-stock-rows-internal accounts #t)))
Expand Down
10 changes: 4 additions & 6 deletions gnucash/report/reports/standard/portfolio.scm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,10 +143,8 @@
"number-cell" value)))
;;(display (format #f "Shares: ~6d " (gnc-numeric-to-double units)))
;;(display units) (newline)
(if price (gnc-price-unref price))
(table-add-stock-rows-internal rest (not odd-row?)))
(begin (if price (gnc-price-unref price))
(table-add-stock-rows-internal rest odd-row?))))))
(table-add-stock-rows-internal rest odd-row?)))))

(set! work-to-do (length accounts))
(table-add-stock-rows-internal accounts #t)))
Expand Down Expand Up @@ -211,7 +209,7 @@
(car price)
(gnc-price-invert (car price))))
(v (gnc-price-get-value the_price)))
(gnc-price-ref (car price))
(gnc:price-ref (car price))
(cons (car price) v))
(cons #f (gnc-numeric-zero)))))
(if price (gnc-price-list-destroy price))
Expand All @@ -223,7 +221,7 @@
(cond
((null? price) (cons #f 0))
(else (let ((p (car price)))
(gnc-price-ref p)
(gnc:price-ref p)
(gnc-price-list-destroy price)
(cons p (gnc-price-get-value p))))))))
((pricedb-nearest)
Expand All @@ -233,7 +231,7 @@
pricedb foreign (time64CanonicalDayTime date)))
(fn (if (and price (> (length price) 0))
(let* ((v (gnc-price-get-value (car price))))
(gnc-price-ref (car price))
(gnc:price-ref (car price))
(cons (car price) v))
(cons #f (gnc-numeric-zero)))))
(if price (gnc-price-list-destroy price))
Expand Down