View Issue Details

IDProjectCategoryView StatusLast Update
0008109Talerexchangepublic2024-01-23 18:55
Reporterfefe Assigned Tofefe  
PrioritynormalSeverityfeatureReproducibilityhave not tried
Status feedbackResolutionopen 
Target Versionpost-1.0 
Summary0008109: feature request: reference counting is not atomic
DescriptionThis 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).
TagsNo tags attached.


child of 0008112 assignedfefe Merchant security review 


Christian Grothoff

2024-01-18 22:28

manager   ~0020910

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?


2024-01-23 13:48

developer   ~0020983

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.

Christian Grothoff

2024-01-23 18:53

manager   ~0020999

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.

Christian Grothoff

2024-01-23 18:55

manager   ~0021000

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.

Issue History

Date Modified Username Field Change
2024-01-18 17:10 fefe New Issue
2024-01-18 17:10 fefe Status new => assigned
2024-01-18 17:10 fefe Assigned To => Christian Grothoff
2024-01-18 22:28 Christian Grothoff Note Added: 0020910
2024-01-18 22:28 Christian Grothoff Assigned To Christian Grothoff => fefe
2024-01-18 22:28 Christian Grothoff Status assigned => feedback
2024-01-18 23:31 Christian Grothoff Relationship added child of 0008112
2024-01-23 13:48 fefe Note Added: 0020983
2024-01-23 18:38 Christian Grothoff Target Version => post-1.0
2024-01-23 18:38 Christian Grothoff Summary reference counting is not atomic => feature request: reference counting is not atomic
2024-01-23 18:53 Christian Grothoff Note Added: 0020999
2024-01-23 18:55 Christian Grothoff Note Added: 0021000
2024-01-23 18:55 Christian Grothoff Severity minor => feature