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

[Kumar Srinivasan]

On 5/7/2013 7:15 AM, Kumar Srinivasan wrote:
> 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.

I think I will leave it as-is, jexec, it appears may not be needed, this 
was added for
Solaris primarily, and I am not sure if this is required anymore (as 
Alan pointed out earlier),
I need to research this, and likely yank out jexec.c completely.

Kumar

>
> 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 <kumar.x.srinivasan at oracle.com> 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 <martinrb at google.com
>>>> <mailto:martinrb at google.com>> 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
>>>>      <kumar.x.srinivasan at oracle.com
>>>>      <mailto:kumar.x.srinivasan at oracle.com>> 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
>>>>>              <kumar.x.srinivasan at oracle.com
>>>>> <mailto:kumar.x.srinivasan at oracle.com>> 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
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>