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

Wasm target #88

Merged
merged 7 commits into from
Sep 4, 2022
Merged

Wasm target #88

merged 7 commits into from
Sep 4, 2022

Conversation

nullrocket
Copy link
Contributor

Added conditional WASM so that the entropy can be properly defined for wasm targets.

Copy link
Owner

@utelle utelle left a comment

Choose a reason for hiding this comment

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

In principle, I'm not against adding additional code to support WebAssembly. However, I find your implementation a bit confusing.

  1. Instead of using an #elif preprocessor instruction you are nesting #if ... #endif constructs. IMHO that makes the code less readable
  2. There are quite a few unnecessary empty lines
  3. Your code uses unconditionally console out (printf). Such output may be useful while debugging, but should not be in production code
  4. Your implementation has actually unreachable code:
#else
  #if defined(__WASM__)
    /* Code that will be executed if symbol __WASM__ is defined */
  #else
    #if defined(__WASM__)
      /* Code unreachable !!! */
      /* Symbol __WASM_ can't be defined here, because we are in*/
      /* the #else branch of the first check for symbol __WASM__ */
    #else
      # error "Secure pseudorandom number generator not implemented for this OS"
    #endif
#endif

I would prefer something like this (if that's what you want/need):

#elif defined(__WASM__)
static size_t entropy(void* buf, size_t n)
{
  if (getentropy(buf, n) == 0)
    return n;
}
#else
# error "Secure pseudorandom number generator not implemented for this OS"
#endif

Conditional define __WASM__ to allow wasm target for clang + wasi-sdk for browser.
Conditional define __WASM__ to allow wasm target for clang + wasi-sdk for browser.
@nullrocket
Copy link
Contributor Author

Sorry about all the noise! Something was wrong with my local git repo, every time i commited changes, it would be a strange merge of an old commit with the new. Fixed now. I thought I was losing my mind!

I do remember why I had the full braces and return here.

static size_t entropy(void* buf, size_t n)

I was seeing this warning and thought I would clear it up.

warning: non-void function does not return a value in all control paths

@nullrocket nullrocket requested a review from utelle September 4, 2022 20:21
Copy link
Owner

@utelle utelle left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning up the code.

Unfortunately, I missed one glitch in the code; function entropy must return a value. That is, this code

if (getentropy(buf, n) == 0)
    return n

has to be changed for example to

  return (getentropy(buf, n) == 0) ? n : 0;

Additionally, the line 401 #endif has to be removed, because there is no corresponding #if.

@utelle
Copy link
Owner

utelle commented Sep 4, 2022

I do remember why I had the full braces and return here.

static size_t entropy(void* buf, size_t n)

I was seeing this warning and thought I would clear it up.

warning: non-void function does not return a value in all control paths

I already commented on the latter, but additionally the keyword static should be dropped. That is, the final code for function entropy should look like this:

extern size_t entropy(void* buf, size_t n);
size_t entropy(void* buf, size_t n)
{
  return (getentropy(buf, n) == 0) ? n : 0;
}

Conditional define __WASM__ to allow wasm target for clang + wasi-sdk for browser.
@nullrocket
Copy link
Contributor Author

Ok, I removed the static keyword, just curious why? There is another definition further up where I believe I got the signature from when I was making the change that is static.

Also just to be sure, your code above is not what I have.

extern size_t entropy(void* buf, size_t n);
size_t entropy(void* buf, size_t n)
{
  return (getentropy(buf, n) == 0) ? n : 0;
}

This is what I have. the extern decl is for getentropy.

extern size_t getentropy(void* buf, size_t n);
size_t entropy(void* buf, size_t n)
{
  return (getentropy(buf, n) == 0) ? n : 0;
}

@utelle
Copy link
Owner

utelle commented Sep 4, 2022

Ok, I removed the static keyword, just curious why? There is another definition further up where I believe I got the signature from when I was making the change that is static.

Ouch, please revert this change. That is, the static keyword should actually be used. Otherwise the symbol will be exported. Sorry for the confusion (it's late in the evening here in Germany 😉 ).

Also just to be sure, your code above is not what I have.
[...]
This is what I have. the extern decl is for getentropy.

extern size_t getentropy(void* buf, size_t n);
size_t entropy(void* buf, size_t n)
{
  return (getentropy(buf, n) == 0) ? n : 0;
}

Yes, your version is correct. Sorry for my lack of concentration.

Copy link
Owner

@utelle utelle left a comment

Choose a reason for hiding this comment

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

I think the PR is now finally correct.

@utelle utelle merged commit eed4afe into utelle:main Sep 4, 2022
utelle added a commit that referenced this pull request Sep 6, 2022
- Prepare release of version 1.5.0
- Upgrade to SQLite version 3.39.3
- Add option to register cipher schemes dynamically
- Eliminate a few compile time warnings
- Add WebAssembly target support (#88, #89)
- Improve error messages from sqlite3_rekey
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