-
-
Notifications
You must be signed in to change notification settings - Fork 11.1k
Closed
Labels
branch: masterApplies to master branchApplies to master branchtriaged: bugThe issue/pr is/fixes a bugThe issue/pr is/fixes a bugtriaged: featureThe issue/pr requests/adds a featureThe issue/pr requests/adds a feature
Milestone
Description
As partly discussed already in #5781 and #6217:
Currently there are some 900 places within crypto/ and ssl/ where in case CRYPTO_malloc() and friends returned NULL an error report with the reason ERR_R_MALLOC_FAILURE is pushed on the ERR queue, rather than the allocation function reporting the error itself. This approach is not only pretty redundant and cumbersome but also more or less inaccurate and error-prone:
- Assuming that the callers of the allocation function(s) are responsible for flagging memory allocation failure, it easily happens that callers simply forget (or do not care) to do so. There are (or at least were) many occurrences of such false negatives.
- Indirect callers of functions that involve memory allocation can also report false positives regarding
ERR_R_MALLOC_FAILUREwhere they misinterpret the reason for obtaining aNULLpointer in case this was not actually due to an malloc failure but due to some other issue (see, e.g.., two instances fixed in improve EVP cipher param diagnostics on unsupported cipher modes and malloc failure #6217). - It also happens that indirect callers of functions that involve memory allocation re-report
ERR_R_MALLOC_FAILUREafter obtaining aNULLpointer though the malloc failure has already been reported by a function in between (see, e.g.., two instances fixed in improve EVP cipher param diagnostics on unsupported cipher modes and malloc failure #6217 caused by the same callsPKCS12err(PKCS12_F_PKCS12_SAFEBAG_CREATE_PKCS8_ENCRYPT, ERR_R_MALLOC_FAILURE);as in the item before). - If a function like
CRYPTO_malloc()does the error reporting itself this relieves all its callers from doing so. - A function like
CRYPTO_malloc()can do the failure/error reporting more accurately than its callers because it knows, in contrast to them, whether the failure is due to memory shortage or due to other issues like unsuitable arguments.
When I asked why CRYPTO_malloc() does not do the error reporting itself I learned about the following reasons.
- An malloc failure could either be because the system is running low on memory, or the call to
CRYPTO_malloc()was erroneous (e.g. because it incorrectly attempts to allocate a vast amount of memory). In the second case this kind of problem would be difficult to find if all you have is the error code inCRYPTO_malloc(). - In the same direction goes the desire to preserve the file/line numbers (and if possible also the function name) of the caller(s) for being able to trace.
- A entirely different issue is that the ERR reporting may in turn need memory allocation (in case its internal queue allocated so far has become full), which may cause a loop in case memory is exhausted.
Here are some thoughts on these issues and suggestions how to deal with them.
- In case of an actual out-of-memory situation it would not help to obtain information about the caller because this is a low-level issue related to the execution environment (rather than to the caller).
- In case of other reasons for malloc failures the malloc functions should give specific error reasons (which the callers cannot do), such as an overly large amount of memory requested or any internal allocation function issue. In case the caller gave an unsuitable argument it may be helpful to know which caller it was, but this is a very general issue that would better be solved more generally (such as, by providing a stack trace as far as possible). When specific value is seen in reporting malloc failures within some calling functions of course this can be done in addition to a more low-level reporting by the malloc function itself.
- In order to remove the obstacle of a potential loop with the ERR queue, it should be possible to adapt its implementation such that there is always an element pre-allocated and reserved for use by malloc functions in case there is actually shortage of memory, such that in case the malloc function adds an
ERR_R_OUT_OF_MEMORYentry there is no need to (recursively) allocate it. When an out-of-memory error has already been reported (and thus the reserved cell has been filled) any further requests to add elements to the ERR queue should be ignored in case the queue buffer is full and would need re-allocation (supposedly this would be acceptable because the entry regarding the out-of-memory error is already in the queue and at least from the library's point of view an out-of-memory condition is fatal anyway).
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
branch: masterApplies to master branchApplies to master branchtriaged: bugThe issue/pr is/fixes a bugThe issue/pr is/fixes a bugtriaged: featureThe issue/pr requests/adds a featureThe issue/pr requests/adds a feature