ports: Raise an error at a negative argument of os.urandom(). - #18833

Code size report:

Reference:  docs/develop/writingtests: Document parameters of run_test.py. [fdb7c0f]
Comparison: tests/extmod/os_urandom.py: Add test for os.urandom. [merge of 6dbabc9]
  mpy-cross:    +0 +0.000% 
   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:   +32 +0.008% PYBV10
      esp32:   +20 +0.001% ESP32_GENERIC
     mimxrt:   +24 +0.006% TEENSY40
        rp2:   +24 +0.003% RPI_PICO_W
       samd:   +24 +0.009% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.42%. Comparing base (fdb7c0f) to head (6dbabc9).
⚠️ Report is 3 commits behind head on master.

@@            Coverage Diff             @@
##           master   #18833      +/-   ##
==========================================
- Coverage   98.42%   98.42%   -0.01%     
==========================================
  Files         174      174              
  Lines       22334    22328       -6     
==========================================
- Hits        21983    21977       -6     
  Misses        351      351              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@robert-hh ,
I notice a test missing for the new error,
I also wonder if it would not be sufficient to raise just a ValueError and omit the text. That should save some bytes.
Or the error should be matched to CPython's .

# 3.11.9
>>> os.urandom(-1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: negative argument not allowed

I had considered both just having ValueError for small flash devices or using the CPython error message and ended with the actual text as compromise for lower complexity.

By the way: I did not change the Windows port because I could not spot the source files. The ports/windows directory does not have many of these. I used to build Windows versions > 7 years ago, and it seems I lost track for that port.

It does have the same issue in 1.27 , but afaikt it uses the code from the Unix port for this, so your fix should work.
Adding a test to CI will help verify. I'll try to find time to test.

The PR repeats the same few lines of code for each port. That looks not nice. I tried to factor out the common code part if mp_os_urandom(),

I think we should do that first. All existing port-specific implementations look like this:

static mp_obj_t mp_os_urandom(mp_obj_t num) {
    mp_int_t n = mp_obj_get_int(num);
    vstr_t vstr;
    vstr_init_len(&vstr, n);
    // port specific code to fill vstr.buf
    return mp_obj_new_bytes_from_vstr(&vstr);
}
static MP_DEFINE_CONST_FUN_OBJ_1(mp_os_urandom_obj, mp_os_urandom);

so could factor that code into extmod/modos.c as something like:

static mp_obj_t mp_os_urandom(mp_obj_t num) {
    mp_int_t n = mp_obj_get_int(num);
    vstr_t vstr;
    vstr_init_len(&vstr, n);
    mp_os_urandom_bytes(n, vstr.buf);
    return mp_obj_new_bytes_from_vstr(&vstr);
}
static MP_DEFINE_CONST_FUN_OBJ_1(mp_os_urandom_obj, mp_os_urandom);

and then each port defines void mp_os_urandom_bytes(size_t n, uint8_t *buf) in its modos.c.

@robert-hh would you like to do that?

and then each port defines void mp_os_urandom_bytes(size_t n, uint8_t *buf) in its modos.c.

If we do this then we can also probably refactor the other places in each port which fill a buffer with random bytes (i.e. similar to the changes in #18834) to call that single helper function, maybe save a little more code size.

If we do this then we can also probably refactor the other places in each port which fill a buffer with random bytes

Yes, good idea. In that case maybe copy what the unix port already has: mp_hal_get_random().

@robert-hh would you like to do that?

@dpgeorge Sure. I had already prepared it, will call that function providing the raw values mp_hal_get_random() and put it into mphalport.c.

@dpgeorge I hav a question about an implementation detail, before changing 11 ports. The unix port defines void mp_hal_get_random(size_t n, void *buf);. Shouldn't it be better void mp_hal_get_random(size_t n, uint8_t *buf);, because that's what the ports do in their implementation of os.urandom().

Shouldn't it be better void mp_hal_get_random(size_t n, uint8_t *buf)

Yes, change the unix port to uint8_t *buf looks better (and I don't see any downsides of that change).

@dpgeorge os.orandom() is now factored out. It ended up with quite a few changed files, but the changes are similar for all ports. The actual random numbers are provided by mp_hal_get_random() of each port. The respective implementation for the ports is moved to mphalport.x or equivalent, except for MIMXRT and SAMD, because theses have more than on instance of mp_hal_get_random() calling another function. That is better kept in a single place.

The PR still has two commits for visibility during the review, but the first one should be squashed. Using mp_hal_get_random() at other places in the code is not yet included. That can be done when the actual state is clean.

The suggested changes are made.

I spotted at least one typical place to use mp_hal_get_ramdom(), which is the function mbedtls_hardware_poll() in mbedtls_port.c. It is changed already for MIMXRT, which used the random number generation from modos already before. There are four more ports where this could be used. Maybe this function can then be moved to another place/file, when it gets port-agnostic.

I spotted at least one typical place to use mp_hal_get_ramdom(), which is the function mbedtls_hardware_poll() in mbedtls_port.c. It is changed already for MIMXRT, which used the random number generation from modos already before. There are four more ports where this could be used. Maybe this function can then be moved to another place/file, when it gets port-agnostic.

Yes, that sounds good. Let's leave it for a follow up PR.

I added a test for os.urandom() which checks negative arguments (as well as 0 and positive values).

Thanks for adding the test. I was wondering whether a .exp file is missing. But it seems that the approach has changed.

I was wondering whether a .exp file is missing. But it seems that the approach has changed.

The approach is still the same. In this case the behaviour matches CPython, so the exp file is generated dynamically as the test is run, via CPython.

Thank you very much.

Thanks @robert-hh! This is indeed a very nice consolidation to have, much appreciated.