View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0008109 | Taler | exchange | public | 2024-01-18 16:10 | 2025-07-11 20:39 |
| Reporter | fefe | Assigned To | Resolution deadline | ||
| Implementer | Reviewer | ||||
| Release signed by | Deployed by | ||||
| Verifier | Communicator | ||||
| Data protection authority notification required | No | ||||
| CSIRT notification required | No | ||||
| Estimated effort (hours) | Actual effort (hours) | 0 | |||
| Fast-track | No | ||||
| Work package | none | ||||
| Priority | normal | Severity | feature | Reproducibility | have not tried |
| Status | acknowledged | Resolution | open | ||
| Target Version | post-2.0 | ||||
| Summary | 0008109: feature request: reference counting is not atomic | ||||
| Description | This is in exchange/src/lib/exchange_api_handle.c: 1836 void 1837 TALER_EXCHANGE_keys_decref (struct TALER_EXCHANGE_Keys *keys) 1838 { 1839 if (NULL == keys) 1840 return; 1841 GNUNET_assert (0 < keys->rc); 1842 keys->rc--; Since the reference counting is serving as a king of synchronization method, potentially across threads, the counting should be atomic. You can use the method used in the janson header for their reference counting. However, if you have two step algorithms like here, more care is needed. The typical implementation will include <stdatomic.h> and use a pattern like this (for decrementing): #include <stdatomic.h> int rc; int foo() { int old; do { old = rc; if (old == 0) { return -1; // handle error } } while (!atomic_compare_exchange_weak(&rc,&old,old-1)); return 0; } Note that you also need this kind of pattern for incrementing, because not checking for numeric overflow when incrementing a refcount leads to memory corruption otherwise (if an attacker could make you overflow and then decref the object, free will be called while references to it still exist). | ||||
| Tags | No tags attached. | ||||
| I'm not sure if we want to go here, as right now we've decided that we basically don't support multi-threading in 99% of the code. The taler-merchant-httpd is also single-threaded, as are all other users of this API. Would you agree that it is adequate to document the "single-threaded use only" restriction in general in the header of the entire API? | |
| I would also like to point out that this battlefield has been fought before, and the industry consensus is that malloc/free/realloc are thread-safe, and that reference counting is an implicit synchronization mechanism and therefore needs to be thread-safe as well. You can certainly document your code to not comply to implicit industry-wide expectations, but dealing with failures in the field because someone didn't get the memo and some crappy mobile app now has some race conditions will far outweigh the costs of making your code thread-safe now. This is basically the same argument as "do not crash the process if something unexpected happens". You can document what you want but people will come to you with expectations, and if there is a failure because the expectations were wrong, it's Taler that will be blamed. "They are not even thread-safe!" "The library crashed our app, can you believe it? I'm going back to Paypal." "I TOLD you we should only use code written in Rust!" In the end it won't matter whether you documented things properly. What will matter is if the end-product can succeed in the market. Don't antagonize your customers by violating their expectations. It's much better to provide an endpoint that exercises all the assertions and potential attacks so that customers can check if their implementation is safe or not. Make it a compliance checkbox that all the error conditions are handled properly. | |
| Added comment in header that the library is expected to be used-single threaded and may abort() on out-of-memory situations. To clarify: this is a *reference* implementation showcasing _how_ one would interact with the REST API that is also used by a single-threaded auto-restarted merchant backend. It is not expected to be used 'everywhere', we have re-implementations in TypeScript and PHP for some of the functions, and I fully expect more language bindings in the future. | |
| To clarify: I'm not against adding the atomic update code, *but* that will still _not_ make the code thread-safe, so the note still applies. | |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2024-01-18 16:10 | fefe | New Issue | |
| 2024-01-18 16:10 | fefe | Status | new => assigned |
| 2024-01-18 16:10 | fefe | Assigned To | => Christian Grothoff |
| 2024-01-18 21:28 | Christian Grothoff | Note Added: 0020910 | |
| 2024-01-18 21:28 | Christian Grothoff | Assigned To | Christian Grothoff => fefe |
| 2024-01-18 21:28 | Christian Grothoff | Status | assigned => feedback |
| 2024-01-18 22:31 | Christian Grothoff | Relationship added | child of 0008112 |
| 2024-01-23 12:48 | fefe | Note Added: 0020983 | |
| 2024-01-23 17:38 | Christian Grothoff | Target Version | => post-1.0 |
| 2024-01-23 17:38 | Christian Grothoff | Summary | reference counting is not atomic => feature request: reference counting is not atomic |
| 2024-01-23 17:53 | Christian Grothoff | Note Added: 0020999 | |
| 2024-01-23 17:55 | Christian Grothoff | Note Added: 0021000 | |
| 2024-01-23 17:55 | Christian Grothoff | Severity | minor => feature |
| 2024-08-12 08:11 | Christian Grothoff | Target Version | post-1.0 => post-2.0 |
| 2025-07-11 20:39 | Christian Grothoff | Assigned To | fefe => |
| 2025-07-11 20:39 | Christian Grothoff | Status | feedback => acknowledged |