[*] Concerns about safety, errors handling and inconsistencies

[raysan5]

This issue is related to a reddit post about errors handling and inconsistencies.

raylib original approach was a hard focused on simplicity, trying to avoid any extra-code that could pollute the functions, leaving the security data checks to the users and expecting the data to be provided to most functions to be valid data.

But raylib has been +11 years in development and in that time some data validation checks have been added here and there resulting in some inconsistencies... and polluting the functions code with some things I originally tried to avoid (i.e early-returns).

Still, I tried to follow some principles in the library:

• Users are the owners of data and valid data should be provided to raylib functions.
• raylib does not use assert() or exit().
• In case of issues with data, priority is logging a warning message and trying to return gracefully (with valid default data or at least something to be validated at user side - i.e. zero-initialized struct)
• raylib does not (usually) use error return codes. (But lately some were added to file-system functions)
• raylib text functions always expect \0 terminated strings, it's up to the user to make sure that happens.
• raylib does not validate memory allocations returned pointers, the chances for a failure on memory allocation are extremely low but, in any case, users can define their own allocator macros.
• In raylib I tried to minimize early returns from functions
• In case some function return an error-code, I prefer to accumulate it into a result variable and use it for checking.

This issue is open to analyze if there could be some of those tentative rules not applied in some functions or if there are some inconsistencies.

[orcmid]

One probably-unhelpful comment:

When "user" is used above, the developer of a raylib-based application is meant. The problem is when that application is distributed to end-users and the application fails in a place where log messages, even if actually produced, are invisible and unknown to the casual user. It is for this situation that defensive programming is valuable.

Also, raylib-internal operations may also need to be defensively programmed, as indicated in some places already, where a problem is not protectable above the API, it being related to under-the-covers failures.

That could also apply to unchecked inputs from above that violate API pre-conditions. Perhaps the pre-conditions should be carefully explicit every and all the time.

The worry should be about defective distributed executables. That's without considering the creation or corruption of an app for malicious purposes.

[sleeptightAnsiC]

EDIT: I'm hiding this. I's a huge and scary wall of text that I'm pretty sure is partially outdated. Some of the information is repeated in later messages.

(I'm the author of the reddit post)

Apologies for not reporting this earlier. I wanted to gather some info/statistics about API before moving this to Github in order to be more specific. Sadly, I had no spare time to do this and thought maybe reddit/discord discussion was more appropriate. Thanks for forwarding the issue @raysan5

I may later report/patch some of those issues mentioned below (I need to test them more deeply). I'm still learning about raylib's API and reading how its code works under the hood. If someone wishes to grab some of these, feel free.

Those are just my observations/concerns. I hope noone gets offended by them. If there is something wrong in this post, let me know and I'll try to reword it.

(Also, writing this for the 2nd time as I closed browser by mistake while testing WASM32...)

---

When "user" is used above, the developer of a raylib-based application is meant.

Yes. Exacly. I'll try to use word "developer" from now on to avoid any confusion.

The worry should be about defective distributed executables. That's without considering the creation or corruption of an app for malicious purposes.

Well, I'm not really concerned with the raylib being used in malicious way, or that the end-users (not developers but users of application) will have hard time understanding what's going on. My main concern and point of view is that the library should allow the developer to decide where to crash, abort, or recover, and API should be consistent and explicit about it.

raylib does not (usually) use error return codes. (But lately some were added to file-system functions)

This is NOT a problem (at leas not to me) because most of underlying calls (deeper than the raylib API itself) set some kind of static error code, e.g. ISO/POSIX LibC sets errno, and OpenGL can be queried for errors with glGetError(), so developer can use those instead. This is a hacky approach but it works. Maybe documentation can mention it but this might be confusing for some.

Users are the owners of data and valid data should be provided to raylib functions.

The problem with this one is that some raylib functions can fail in multiple cases. Validating all those cases would often just double the work that the underlying call can/will redo anyway. See the point 3 and 4 down below.

---

My main concerns (partially mentioned in Reddit post):

1. Raylib does not allow to recover from failure of memory allocation. [EDIT] After thinking about this for a while, and after reading source tree of few raylib's dependencies, it seems that GLFW also does not try to recover from malloc failure. So let's just make sure that RL_MALLOC/RL_CALLOC/RL_FREE is being used in those dependencies and we should be fine. This might be tricky on platforms like Win32 and Wasm32 but oh well...

2. Inconsistencies in API. I gave an example about this occurring in RTEXT, where a lot of text manipulating functions check the string buffer before dereferencing NULL (e.g. MeasureTextEx) while others do not (e.g. TextToInteger). This makes some of those functions invoke UB (crash / memory corruption) while others silently fail and return semi-valid output.

3. Silent failures which result in output that looks quite right but developer is unable to tell weather function call succeed or not. For example, GetFileLength can fail in two cases: if you pass invalid file path to it, the underlying call to fopen will fail, AND when file length is too big for signed 32bit integer (there is a check preventing overflow). In both those cases function returns 0 which is a valid file length. In case of overflow it also logs a warning (probably should be an error) but in case of failing to open the file there is no warning/error. Most libraries would return -1 in those scenarios.

4. Some raylib calls make sub-calls that can fail internally but the top-call does not check for the failure of sub-call. For example, LoadShader makes lots of sub-calls and many of them can fail. There is a function called IsShaderValid that could be helpful here, but for example, when passing two invalid files names to LoadShader, the returned shader would still result in evaluating IsShaderValid to true (bug? I just checked it and was surprised). Another example: InitWindow can fail internally and there is a function that let's developer check if window was initialized successfully (IsWindowReady) but InitWindow would already crash the application giving no chance to recover. In those cases it's probably easier for developer to re-implement the whole function than validate the input.

[bluesillybeard]

I figured I would add something to the discussion, however useless it may be.

I'm developing a library roughly similar to Raylib. (not released or open yet), It uses more or less the opposite approach:

• Users (of the library) are expected to provide valid data, and they are immediately informed when they don't
• Every function that has any chance of something going wrong returns an error code
• Strings can be either null terminated, or the user can provide a length
• Allocations are never shared between the user and the library. The user has their heap, and the library has its heap.

This does mean data is frequently copied where a simple pointer would work. But I've had enough trouble with libraries that expose their internal allocations and/or expect the user to expose theirs.

My library separates errors into three categories:

• User errors: asserts with additional information about what the user did wrong and how to fix it
• Assert errors: asserts that indicate something is wrong with the library itself
• External errors: errors like a missing OpenGL driver or something

Memory allocations are not checked. If you can't allocate memory, you're probably screwed anyway.

User errors and Assert errors are asserts, in the sense that they trigger the debugger and exit the program.

An external error causes the function it occurred in to return a bad return code, and also triggers an error callback with additional information. In fact, all asserts trigger an error callback, so the user can do any of their own debugger / assert logic.

Finally, all error checking is entirely optional. The user doesn't have to look at the error return code. Neither do they have to set an error callback. All asserts and error checking can be enabled or disabled with some macro definitions, which are of course exposed in the build system with sensible defaults.

This is just my approach - honestly I'm not sure how much of this is useful for Raylib.

[raysan5]

@bluesillybeard Thanks for sharing, definitely a different approach than raylib one.

[sleeptightAnsiC]

Linking with old issue that was partially mentioning this topic: #1420 "Errors are hidden in the Library and not getting up in the Application Layer" (btw, some of those issues will be fixed soon)

[sleeptightAnsiC]

Something I found a bit strange is that all Font-related functions in rtext.c can operate on a malformed Font when isGpuReady is false. There is always a check for this condition but there are no warnings about it. This means that if you try to LoadFont() before InitWindow(), it silently returns a broken Font.

@raysan5 Are there any cases where Font can be safely used without GPU ?

$ cd raylib/src
$ grep -rn 'if.*isGpuReady'
rtext.c:260:    if (isGpuReady) defaultFont.texture = LoadTextureFromImage(imFont);
rtext.c:316:    if (isGpuReady) UnloadTexture(defaultFont.texture);
rtext.c:370:    if (isGpuReady)
rtext.c:498:    if (isGpuReady) font.texture = LoadTextureFromImage(fontClear); // Convert processed image to OpenGL texture
rtext.c:567:        if (isGpuReady) font.texture = LoadTextureFromImage(atlas);
rtext.c:958:        if (isGpuReady) UnloadTexture(font.texture);
rtext.c:1078:    byteCount += sprintf(txtData + byteCount, "    if (isGpuReady) font.texture = LoadTextureFromImage(imFont);\n");
rtext.c:1289:    if ((isGpuReady && (font.texture.id == 0)) ||
rtext.c:2272:    if (isGpuReady) font.texture = LoadTextureFromImage(fullFont);
rtext.c:2314:    if (isGpuReady && (font.texture.id == 0))

[sleeptightAnsiC]

Mentioned by @karl-zylinski at https://discord.com/channels/426912293134270465/474679851018616833/1353860008374439946

It would be great if LoadTexture checked if the window has been created, and if not print an error (or panic). Right now loading a texture before window creation makes the program crash when you try to use it, and it can  be hard to understand why.

Lot's of people accidentally do this in the global file scope in Odin (outside of any procedure):
some_texture := rl.LoadTexture("picture.png")

Which makes it happen on program startup, before main has run. And then their program crashes in, for them, mysterious ways. It even says that it loaded the texture!

This part is interesting: program crashes [...] in mysterious ways. It even says that it loaded the texture!

This is how this issue looks like:

~/raylib/build/examples (master) $ ./textures_background_scrolling 
INFO: FILEIO: [resources/cyberpunk_street_background.png] File loaded successfully
INFO: IMAGE: Data loaded successfully (512x192 | R8G8B8A8 | 1 mipmaps)
Segmentation fault (core dumped)

[3xau1o]

Would like to get a solid and consistent library like SDL with simplicity of raylib

[LainLayer]

Currently, textures.c has a lot of checks like this in many functions:

if ((image->data == NULL) || (image->width == 0) || (image->height == 0)) return;

Users are the owners of data and valid data should be provided to raylib functions.

If the intended developer behavior is to rely on IsImageValid, then this check should be omitted, and it should segfault if the image data is NULL. Using a silent failure return here could lead to chasing an annoying bug which will crash later on in the code.

I believe that passing an image with data = NULL should crash instantly.