raspberrypi: SSLSocket: raise OSError when appropriate

[jepler]

Rather than returning the negative error value.

This is intended to close #7606, though I did not test with mqtt (EDIT by @dhalbert: see second post). Instead, I created a simple standalone test program:

import wifi, socketpool, ssl, time
#wifi.radio.connect(<omitted>)
import socketpool
socket = socketpool.SocketPool(wifi.radio)
ctx = ssl.create_default_context()

b = bytearray(8)

s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
sss = ctx.wrap_socket(s, server_hostname='example.com')
sss.connect(('example.com', 443))
sss.setblocking(False)
r = sss.recv_into(b)
print(r, b)  # prints 4294967285 which is -11 as unsigned
sss.close()

Before the change, r was the out of range value 4294967285. After the change, the recv_into call raises OSError instead.

This is comparable to the behavior on standard Python, though an SSLWantReadError is raised instead.

The original (mis)behavior seems to match what was uncovered deep inside minimqtt by adding logging:

370.578: DEBUG - PKT: _sock_exact_recv: recv_len = 4294967285

Closes: #7606

[jepler]

I also tested with mqtt on hivemq.cloud with the following program adapted from the original test program on #7606.

# ______________________________________________________ TEST CODE for LOCAL REMOTE broker TLS problem

import os
import time
import microcontroller
import adafruit_minimqtt.adafruit_minimqtt as MQTT
import ssl
import adafruit_logging as logging

# NOTE: relies on web workflow being configured in settings.toml!

MQTT_broker = os.getenv('MQTT_BROKER')
MQTT_port = os.getenv('MQTT_PORT', 8883)
MQTT_username = os.getenv('MQTT_USERNAME')
MQTT_password = os.getenv('MQTT_PASSWORD')

MQTT_topic = "fnord"
MQTT_hello = "hello world"

DIAG = True # False # ___________________________________ global print disable switch / overwritten by console [D][enter]

def dp(line=" ", ende="\n"):
    if DIAG : print(line, end=ende)

import socketpool
from ipaddress import ip_address
import wifi

pool = socketpool.SocketPool(wifi.radio)

### MQTT Code ###
# Define callback methods which are called when events occur
# pylint: disable=unused-argument, redefined-outer-name
def connect(mqtt_client, userdata, flags, rc):
    # This function will be called when the mqtt_client is connected
    # successfully to the broker.
    dp("___ Connected to MQTT Broker!")
    dp("___ Flags: {0} RC: {1}".format(flags, rc))


def disconnect(mqtt_client, userdata, rc):
    # This method is called when the mqtt_client disconnects
    # from the broker.
    dp("___ Disconnected from MQTT Broker!")


def subscribe(mqtt_client, userdata, topic, granted_qos):
    # This method is called when the mqtt_client subscribes to a new feed.
    dp("___ Subscribed to {0} with QOS level {1}".format(topic, granted_qos))


def unsubscribe(mqtt_client, userdata, topic, pid):
    # This method is called when the mqtt_client unsubscribes from a feed.
    dp("___ Unsubscribed from {0} with PID {1}".format(topic, pid))


def publish(mqtt_client, userdata, topic, pid):
    # This method is called when the mqtt_client publishes data to a feed.
    dp("___ Published to {0} with PID {1} ".format(topic, pid))


def message(client, topic, message):
    #global Va,Vb,Aa,Ab,Wa,Wb # here overwritten
    # Method called when a client's subscribed feed has a new value.
    dp("___ New message on topic {0}: {1}".format(topic, message))
    # here evaluate REMOT COMMANDS

### END MQTT functions ###


# Set up a MiniMQTT Client !! from adafruit example NOT use client name?
# https://docs.circuitpython.org/projects/minimqtt/en/latest/api.html
# client_id (str) – Optional client identifier, defaults to a unique, generated string.
dp("___+++ setup MQTTclient")
dp("___++++ use TLS")
mqtt_client = MQTT.MQTT(
    broker=MQTT_broker,
    port=MQTT_port,
    username=MQTT_username,
    password=MQTT_password,
    socket_pool=pool,
    ssl_context=ssl.create_default_context(),
    is_ssl=True,
)
mqtt_client.enable_logger(logging, log_level=logging.DEBUG)

# Connect callback handlers to mqtt_client
mqtt_client.on_connect = connect
mqtt_client.on_disconnect = disconnect
mqtt_client.on_subscribe = subscribe
mqtt_client.on_unsubscribe = unsubscribe
mqtt_client.on_publish = publish
mqtt_client.on_message = message

try:
    dp("___ Attempting to connect to %s" % mqtt_client.broker)
    mqtt_client.connect()
    MQTTok = True # _______________________________ used later for publish
except Exception as e:
    print("Error: MQTT connect\n", str(e))
    MQTTok = False

try:
    dp("___ Publishing to %s" % MQTT_topic) # _____ only master topic should gets that
    dp(MQTT_hello)
    mqtt_client.publish(MQTT_topic,MQTT_hello )
    MQTTok = True # _______________________________ used later for publish
except Exception as e:
    print("Error: MQTT publish hello\n", str(e))
    #MQTTok = False

    mqtt_topic_tune = MQTT_topic + "/set"
    mqtt_client.subscribe(mqtt_topic_tune)

    try:
        # setup subscribe
        dp("___ Subscribing to %s tuning" % mqtt_topic_tune)
        mqtt_client.subscribe(mqtt_topic_tune)
    except ZeroDivisionError as e:
        print("Error: MQTT subscribe tuning\n", str(e))
        raise e
        #MQTTok = False

    dp("___ mqtt_topic: "+mqtt_topic)

while True :
    try:
        mqtt_client.loop() # ______________________________ now we see the subscribed tuning come through
    except Exception as e:
        print("Error: MQTT loop\n", str(e))
        MQTTok = False
        #time.sleep(10) # __________________________________ broker reboot expected
        #microcontroller.reset() # _________________________ try reboot case broker recovered

    time.sleep(0.5)

[dhalbert]

common_hal_socketpool_socket_recv_into() returns an int, and similar for other return values in that file.

But common_hal_ssl_sslsocket_recv_into() returns an mp_uint_t, and similarly for ...send(). I think it is inconsistent that the SSL versions return unsigned ints. Either all the common_hal routines like this should never return ints (which might be negative error values), and always throw, or they should all return ints. This is how we got into trouble.

[jepler]

Would you like me to change that here (in 8.0.x) or in a separate PR (for main)?

[dhalbert]

Would you like me to change that here (in 8.0.x) or in a separate PR (for main)?

We can do a separate PR for main because it's churn. Do you agree they should be regularized, or is there some reason they are not?

[jepler]

No, I don't know if there's a reason the return value types are what they are

[dhalbert]

Thank you for the revamped fix!

[jepler]

Thanks to @DavePutz and others on 7606 for the sleuthing!