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

Made srand(...) and rand() platform independent #711

Merged
merged 1 commit into from
Aug 16, 2023

Conversation

parmi93
Copy link
Contributor

@parmi93 parmi93 commented Jun 13, 2023

The srand(...) and rand() functions are not available on all embedded platforms, so they were replaced with lwm2m_srand(...) and lwm2m_rand() to make these calls platform independent.

@parmi93 parmi93 force-pushed the rand_platform_independent branch 2 times, most recently from 19b5647 to e5dbff6 Compare June 16, 2023 12:38
@parmi93
Copy link
Contributor Author

parmi93 commented Jun 16, 2023

I noticed that the messages of all previous commits are also checked (which kept failing), so I deleted the previous commits, leaving only the last commit in the history.
Can you please start compliance checks again? I hope this works

@mlasch
Copy link
Contributor

mlasch commented Jun 29, 2023

Can you share on which (embedded-)platform you are running Wakaama? It looks like you still include stdlib.h, so I assume some kind of standard C library is available. srand() and rand() are C89/C90 standard (ISO/IEC 9899:1990) and newer.

I'm wondering what's the best way how to deal with exotic platforms in general. I don't think it's a good practice to re-implement and wrap standard C functions. In my opinion we should assume that we can rely at least on the ISO C standard library (and POSIX?). However the project does not state any minimal requirements yet. Might be worth a discussion #494.

If (s)rand() is not available on your platform you might implement your own rand() function without the wrapper? I guess the same goes for malloc(), free() etc..

@parmi93
Copy link
Contributor Author

parmi93 commented Jun 29, 2023

Sorry it's my mistake, I shouldn't have said it's not available.
Actually the pseudorandom number generators functions srand(int seed) and rand() are available and work as expected by the standard.

The problem with this pseudo random generator is that it relies on the seed, so given a seed, the rand() will always generate the same pseudorandom number.
So if the platform is not able to generate a different seed at each boot, then the sequence of pseudorandom numbers generated will always be the same.

In this specific case Wakaama uses the timestamp returned by lwm2m_gettime() as seed, the problem is that in various embedded platforms the clock (RTC) is not available, or if the hardware (RTC) to keep the time is available, it can happen that the backup battery is flat, and therefore every boot the timestamp will start from the same number.

At the moment, at every boot, I connect to an NTP server (time.google.com) to synchronize the clock, but the problem remains, because it can happen that I am under a network where the rules prevent me from accessing certain IP addresses (e.g. China?), or the NTP service no longer works...

For this reason, manufacturers usually provide proprietary functions/APIs that generate a real random number (as in my case).

I am working with cellular modem (BG95-M5) which runs with ThreadX, a non-POSIX platform.

Another solution could be the following:

lwm2m_context_t * lwm2m_init(void * userData)
{
    lwm2m_context_t * contextP;

    LOG("Entering");
    contextP = (lwm2m_context_t *)lwm2m_malloc(sizeof(lwm2m_context_t));
    if (NULL != contextP)
    {
        memset(contextP, 0, sizeof(lwm2m_context_t));
        contextP->userData = userData;
        srand((int)lwm2m_seed());
        contextP->nextMID = rand();
    }

    return contextP;
}

@LukasWoodtli
Copy link
Contributor

Even if I like the idea to build platform independent software. I would try to reduce the customization possibilities in the code.

I would prefer to leverage the possibilities of the toolchain (compiler, linker...) or the OS (loader) for such customization. These tools have all we need and it reduces some unneccessary complexety of our code.

If you want to use your custom srand() funciton you can just implement it in one of your C files.

void srand (unsigned int seed) {
    (void)seed;
    fprintf(stdout, "Hello from srand");
}

int main(int argc, char *argv[])
{
// ...
}

The linker will pick your implementation instead the one from the standard library.

@parmi93
Copy link
Contributor Author

parmi93 commented Jun 29, 2023

I understand your point of view.

But I think the problem is still there.
This line of code is implicitly assuming that the system clock is synchronized:
https://github.com/eclipse/wakaama/blob/514659414c792f8c252782af63551b6d09704a7b/core/liblwm2m.c#L71
I don't think Wakaama can take this for granted, because even under POSIX (Linux) system this might not be true (e g. backup battery is flat).
At most we can take for granted that the timestamp returned by lwm2m_gettime() is incremented by the system every second.

Perhaps it would be better to replace with:

srand((int)lwm2m_seed());

And let the user define the lwm2m_seed() function in the platform.c file?

@LukasWoodtli
Copy link
Contributor

I see your point. So probably we need a possibility to customise srand().
Another idea would be to provide a second argument to lwm2m_init with the seed. But I don’t know if that is feasible.

@mlasch
Copy link
Contributor

mlasch commented Jun 29, 2023

@parmi93 all valid points, thanks for clarifying. I like your proposed solution to have a custom seed function.

Perhaps it would be better to replace with:

srand((int)lwm2m_seed());

And let the user define the lwm2m_seed() function in the platform.c file?

@parmi93
Copy link
Contributor Author

parmi93 commented Jul 6, 2023

I need a hint, what can I write in the platform.c file as an example for generating the seed?

int lwm2m_seed(void)
{
    //what should I return?
}

I was thinking of something that works on POSIX (Linux) platforms, and fails to compile on non-Linux platforms, so anyone implementing this library on a non-Linux platform would have to look here and fix this error by providing a reliable seed.
Any suggestion?

@mlasch
Copy link
Contributor

mlasch commented Jul 20, 2023

I would suggest to keep time(NULL) as a default seed source due to the lack of a better option. Using lwm2m_seed() instead of lwm2m_gettime is much clearer.

@parmi93
Copy link
Contributor Author

parmi93 commented Jul 26, 2023

Commits squashed and commit message subject line changed.

core/liblwm2m.c Outdated Show resolved Hide resolved
@mlasch
Copy link
Contributor

mlasch commented Jul 28, 2023

The changes look good. Now the CI is still complaining about formatting. You can use clang-format to auto-format your changes. e.g.

git clang-format HEAD~1

The lwm2m_seed() function is used to obtain a seed for random number
generation.
The implementation of the lwm2m_seed() function is up to the user.
@parmi93
Copy link
Contributor Author

parmi93 commented Aug 15, 2023

Code formatted with clang-format.

@mlasch mlasch merged commit 9433484 into eclipse-wakaama:master Aug 16, 2023
30 checks passed
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.

3 participants