Re: RFR: JDK-8013736: [launcher] cleanup code for correctness

Tue, 07 May 2013 07:17:33 -0700 

On 5/6/2013 7:38 PM, Martin Buchholz wrote:

This looks good.

There's one check I found confusing.

175     if (alen <= 0 || alen > INT_MAX / sizeof(char *)) {

Not that it matters much, but I'm not sure exactly what you're trying
to check here.
If you're trying to check that argc+2 doesn't overflow INT_MAX, I
would do that directly in the original argc check.


Making sure alen won't overflow, I will look into your suggestion.

Kumar


If you're trying to check that alen won't overflow, I would check something like
(argc+2) < SIZE_MAX/sizeof(const char *)

On 5/6/13, Kumar Srinivasan <[email protected]> wrote:

Here is the modified webrev:
http://cr.openjdk.java.net/~ksrini/8013736/webrev.1/

delta webrev to the last webrev:
http://cr.openjdk.java.net/~ksrini/8013736/webrev.1/webrev.delta/index.html

I added the do { ..... } while (0) pattern to all the launcher's macro
defs,
I also addressed Alan's comments.

I know that the native unpacker uses macros which will need this pattern
I will fix that separately.

Alan, do you want me to search and file bugs on other jdk components ?

Thanks
Kumar




Oops.  Of course, that shouldn't have the trailing semicolon

#define NULL_CHECK_OR_RETURN(ncor_pointer_to_check,     \
      ncor_failure_return_value) \
   do {                                  \
     if ((ncor_pointer_to_check) == NULL) {              \
JLI_ReportErrorMessage(JNI_ERROR);                \
       return ncor_failure_return_value;                 \
     }                                 \
   } while(0)



On Thu, May 2, 2013 at 11:16 AM, Martin Buchholz <[email protected]
<mailto:[email protected]>> wrote:

     What would Martin actually do?

     #define NULL_CHECK_OR_RETURN(ncor_pointer_to_check,     \
                ncor_failure_return_value) \
       do {                                        \
         if ((ncor_pointer_to_check) == NULL) {              \
     JLI_ReportErrorMessage(JNI_ERROR);                \
           return ncor_failure_return_value;                 \
         }                                       \
       } while(0);




     On Thu, May 2, 2013 at 10:45 AM, Kumar Srinivasan
     <[email protected]
     <mailto:[email protected]>> wrote:

         On 5/2/2013 10:17 AM, Martin Buchholz wrote:

         This is global fix creep, but ...

         :(



         these macros are also found in the hotspot sources.
         I would rewrite all the macros in the jdk to adopt the
         blessed style
         do { ... } while(0)
         and remove all comments in the jdk of the form
          /* next token must be ; */
         If you want a macro that does nothing at all, you should
         define it
         do {} while (0)
         instead of defining it to the empty string.

         I am not following, could you be more explicit on how this
         applies to

         -#define NULL_CHECK0(e) if ((e) == 0) { \
         +#define NULL_CHECK_RV(e, rv) if ((e) == 0) { \
               JLI_ReportErrorMessage(JNI_ERROR); \
         -    return 0; \
         +    return rv; \
             }

         -#define NULL_CHECK(e) if ((e) == 0) { \
         -    JLI_ReportErrorMessage(JNI_ERROR); \
         -    return; \
         -  }
         +#define NULL_CHECK0(e) NULL_CHECK_RV(e, 0)

         +#define NULL_CHECK(e) NULL_CHECK_RV(e, )
         +




             I would also be inclined to change
             == 0
             to
             == NULL


         Yes will take care of this, as well as Alan suggestion added a
         check for malloc return.



             This seems like another occasion to use the weird

             do { ... } while(0)

             trick to make the macro behave more like a statement.

             I might obfuscate the macro parameters to make collisions
             less likely, e.g. e => N_C_RV_e


         You want me to rename the macro parameter e to N_C_RV_e ? or
         something else
         say ncrve to avoid collision ?

         Kumar






             On Wed, May 1, 2013 at 12:33 PM, Kumar Srinivasan
             <[email protected]
             <mailto:[email protected]>> wrote:

                 Hi,

                 Please review simple fixes for code correctness in
                 the launcher.

                 http://cr.openjdk.java.net/~ksrini/8013736/webrev.0/
                 <http://cr.openjdk.java.net/%7Eksrini/8013736/webrev.0/>

                 Thanks
                 Kumar

Reply via email to