Return NULL early in context_preallocated_create if flags invalid #840

1. 
   real-or-random commented at 1:47 pm on October 26, 2020: contributor

   If the user passes invalid flags to _context_create, and the default illegal callback does not abort the program (which is possible), then we work with the result of malloc(0), which may be undefined behavior. This violates the promise that a library function won’t crash after the illegal callback has been called.

   This commit fixes this issue by returning NULL early in _context_create in that case.

2. in src/secp256k1.c:141 in 246b081d68 outdated

   143-
   144     secp256k1_ecmult_context_init(&ret->ecmult_ctx);
   145     secp256k1_ecmult_gen_context_init(&ret->ecmult_gen_ctx);
   146 
   147+    /* Flags have been checked by secp256k1_context_preallocated_size. */
   148+    VERIFY_CHECK((flags & SECP256K1_FLAGS_TYPE_MASK) != SECP256K1_FLAGS_TYPE_CONTEXT);
   

   ---

   ---

   

   sipa commented at 3:17 am on October 27, 2020:

   I think you mean == here.
3. 
   sipa commented at 3:18 am on October 27, 2020: contributor

   Concept ACK, but the newly introduced VERIFY_CHECK is wrong, causing tests to fail.
4. 
   jonasnick commented at 10:49 am on October 27, 2020: contributor

   Looks good to me. Local tests pass with the != -> == fix.
5. Return NULL early in context_preallocated_create if flags invalid

   If the user passes invalid flags to _context_create, and the default
   illegal callback does not abort the program (which is possible), then we
   work with the result of malloc(0), which may be undefined behavior. This
   violates the promise that a library function won't crash after the
   illegal callback has been called.
   
   This commit fixes this issue by returning NULL early in _context_create
   in that case.

   ebfa2058e9
6. real-or-random force-pushed on Oct 27, 2020
7. 
   real-or-random commented at 2:03 pm on October 27, 2020: contributor

   Fixed…
8. 
   sipa commented at 6:11 pm on October 27, 2020: contributor

   ACK ebfa2058e9cc2999dada47d2f1e1e5c0f4bcf619
9. 
   jonasnick commented at 6:55 pm on October 27, 2020: contributor

   ACK ebfa2058e9cc2999dada47d2f1e1e5c0f4bcf619
10. 
    gmaxwell commented at 8:37 pm on October 27, 2020: contributor

    Should the header at secp256k1_preallocated.h indicate that this can return a null pointer on error? (I guess it could before too, but it appears to be undocumented.)
11. 
    real-or-random commented at 9:28 am on October 28, 2020: contributor

    Should the header at secp256k1_preallocated.h indicate that this can return a null pointer on error? (I guess it could before too, but it appears to be undocumented.)

    The same is true for the the normal _context_create, which calls _context_preallocated_create (this is the case when we get malloc(0). This issue has also been brought up in #783. What is true is that we say the return values of the function are undefined if the illegal callback fires. And this is enough IMO.

    But the docs are really difficult to understand here. We give a promise that the illegal callback “will only trigger for violations that are mentioned explicitly in the header.” and this is not really mentioned (or at least) it’s not very implicit. We may want to add a sentence for each function can calls the illegal callback but I don’t know if we want to commit on this… I suggest to discuss this in #783.

12. 
    gmaxwell commented at 6:27 pm on October 28, 2020: contributor

    K. Just thought I’d mention it.
13. jonasnick merged this on Oct 30, 2020
14. jonasnick closed this on Oct 30, 2020

    ---

15. jasonbcox referenced this in commit 9acf260b37 on Oct 31, 2020
16. deadalnix referenced this in commit 649453336e on Nov 1, 2020

Contributors
real-or-random sipa jonasnick gmaxwell