Skip to content

Enhancement - Standardize Error Returns for Failure Paths#483

Merged
Donnie-Ice merged 3 commits into
nasa:devfrom
porfanid:Error-Returns
Aug 20, 2025
Merged

Enhancement - Standardize Error Returns for Failure Paths#483
Donnie-Ice merged 3 commits into
nasa:devfrom
porfanid:Error-Returns

Conversation

@porfanid
Copy link
Copy Markdown
Contributor

@porfanid porfanid commented Jul 23, 2025

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  • Does your submission pass tests?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?

Explanation of Changes

This pull request addresses inconsistent error handling across the CryptoLib codebase by standardizing functions to return int32_t with proper CRYPTO_LIB_* error codes instead of void or inconsistent return types.

Problem Addressed:
Several functions in the codebase that could encounter errors (memory allocation failures, parameter validation, etc.) were returning void or not properly checking/propagating errors from sub-functions, making debugging and error handling more challenging.

Changes Made:

  1. Core Configuration Functions (src/core/crypto_config.c):

    • crypto_deep_copy_string(): Changed from returning char* to int32_t with output parameter pattern
    • Crypto_Local_Config(): Changed from void to int32_t
    • Crypto_Local_Init(): Changed from void to int32_t
    • Crypto_Calc_CRC_Init_Table(): Changed from void to int32_t
    • Added proper error checking for key_if->key_init() and mc_if->mc_initialize() calls
  2. Security Association Functions (src/sa/internal/sa_interface_inmemory.template.c):

    • update_sa_from_ptr(): Changed from void to int32_t with parameter validation
    • sa_populate(): Changed from void to int32_t with error propagation
  3. Header Updates (include/crypto.h):

    • Updated function declarations to match new int32_t return types
    • Updated crypto_deep_copy_string() signature to use output parameter pattern

Error Handling Improvements:

  • Memory allocation safety with malloc failure detection
  • Parameter validation with specific error codes
  • Error propagation from sub-functions
  • Consistent CRYPTO_LIB_* error code usage
  • Graceful NULL handling where appropriate

Why Include These Changes:

  • Improves robustness and debuggability of error handling
  • Maintains full backward compatibility
  • Provides applications with proper error detection capabilities
  • Follows established error handling patterns in the codebase
  • Enhances memory safety and parameter validation

How do you test these changes?

Testing Approach:

  1. Unit Test Validation: All existing unit tests continue to pass, ensuring backward compatibility is maintained
  2. Error Path Testing: Functions now properly detect and report various error conditions:
    • NULL pointer validation (returns CRYPTO_LIB_ERR_NULL_BUFFER)
    • Memory allocation failures (returns CRYPTO_LIB_ERROR)
    • Invalid SPI bounds (returns CRYPTO_LIB_ERR_SPI_INDEX_OOB)
  3. Integration Testing: Configuration functions fail fast with meaningful error codes when invalid parameters are provided
  4. Memory Safety Testing: malloc failures are properly detected and reported instead of causing undefined behavior

Example Test Cases:

// Test error detection in crypto_deep_copy_string
char* result;
int32_t status = crypto_deep_copy_string(NULL, &result);
assert(status == CRYPTO_LIB_ERR_NULL_BUFFER);

// Test proper success case
status = crypto_deep_copy_string("test", &result);
assert(status == CRYPTO_LIB_SUCCESS);
assert(strcmp(result, "test") == 0);
free(result);

// Test configuration function error propagation
status = Crypto_Local_Config();
// Now returns meaningful error codes instead of void

The changes significantly improve the robustness and debuggability of CryptoLib's error handling while maintaining full backward compatibility.

Solves #482

Copilot AI and others added 3 commits July 23, 2025 15:23
…nctions

Co-authored-by: porfanid <19299306+porfanid@users.noreply.github.com>
…tialize

Co-authored-by: porfanid <19299306+porfanid@users.noreply.github.com>
@jlucas9 jlucas9 self-assigned this Jul 24, 2025
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 34.28571% with 92 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.50%. Comparing base (478c31f) to head (1876de0).
⚠️ Report is 91 commits behind head on dev.

Files with missing lines Patch % Lines
src/core/crypto_config.c 32.53% 55 Missing and 30 partials ⚠️
src/sa/internal/sa_interface_inmemory.template.c 50.00% 4 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #483      +/-   ##
==========================================
- Coverage   82.00%   81.50%   -0.50%     
==========================================
  Files          39       39              
  Lines       11945    12174     +229     
  Branches      973     1044      +71     
==========================================
+ Hits         9795     9923     +128     
- Misses       1778     1847      +69     
- Partials      372      404      +32     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Donnie-Ice Donnie-Ice assigned Donnie-Ice and unassigned jlucas9 Aug 12, 2025
@Donnie-Ice Donnie-Ice merged commit b7ae4a9 into nasa:dev Aug 20, 2025
21 of 25 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.

5 participants