[libgfortran,patch] Zero-length strings messed up in library (PR33079)

[libgfortran,patch] Zero-length strings messed up in library (PR33079)

[François-Xavier Coudert]

Hi all,

When TRIM, MIN and MAX result is a zero-length string, the library
functions actually make it a NULL pointer instead "" (ie a pointer to
'\0'). The MIN and MAX case error is mine, because I copied the code
from TRIM when I wrote string_minmax(), while the error in
string_trim() has probably been there for a long time. This patch
fixes both. I could not see any other library routine which suffered
from this problem.

Regtested on x86_64-linux, comes with a testcase. OK for mainline?

FX



:ADDPATCH libgfortran:

2007-08-16  Francois-Xavier Coudert  <fxcoudert@gcc.gnu.org>

        PR fortran/33079
        * intrinsics/string_intrinsics.c (string_trim, string_minmax):
        Fix the zero-length result case.


Index: intrinsics/string_intrinsics.c
===================================================================
--- intrinsics/string_intrinsics.c      (revision 127490)
+++ intrinsics/string_intrinsics.c      (working copy)
@@ -167,16 +167,21 @@ string_trim (GFC_INTEGER_4 * len, void *
     }
   *len = i + 1;

-  if (*len > 0)
+  if (*len == 0)
+    {
+      /* A zero-length Fortran string is "".  */
+      char * tmp = internal_malloc_size (1);
+      tmp[0] = '\0';
+      *dest = tmp;
+    }
+  else
     {
       /* Allocate space for result string.  */
       *dest = internal_malloc_size (*len);

-      /* copy string if necessary.  */
+      /* Copy string if necessary.  */
       memmove (*dest, src, *len);
     }
-  else
-    *dest = NULL;
 }


@@ -403,14 +408,18 @@ string_minmax (GFC_INTEGER_4 *rlen, void
     }
   va_end (ap);

-  if (*rlen > 0)
+  if (*rlen == 0)
+    {
+      /* A zero-length Fortran string is "".  */
+      char * tmp = internal_malloc_size (1);
+      tmp[0] = '\0';
+      *dest = tmp;
+    }
+  else
     {
       char * tmp = internal_malloc_size (*rlen);
       memcpy (tmp, res, reslen);
       memset (&tmp[reslen], ' ', *rlen - reslen);
       *dest = tmp;
     }
-  else
-    *dest = NULL;
 }
-


! { dg-do run }
  character(len=1) :: s
  character(len=0) :: s0
  s = " "
  s0 = ""
  call bar ("")
  call bar (s)
  call bar (s0)
  call bar (trim(s))
  call bar (min(s0,s0))
contains
  subroutine bar (s)
    character(len=*), optional :: s
    if (.not. present (S)) call abort
  end subroutine bar
end

Re: [libgfortran,patch] Zero-length strings messed up in library (PR33079)

[Tobias Burnus]

:REVIEWMAIL:

François-Xavier Coudert wrote:
> Regtested on x86_64-linux, comes with a testcase. OK for mainline?
OK (with a proper changelog for the test case ;-).

Good that you remembered to change minval/maxval as well!

Tobias,
who hates the anti-spam measurement of his provider of rejecting emails
with temporarily unavailable before accepting them at the second try;
while this cuts down the spam, it delays at least some of the emails by
 hours.

Re: [libgfortran,patch] Zero-length strings messed up in library (PR33079)

[Tobias Schlüter]

François-Xavier Coudert wrote:
> -  if (*len > 0)
> +  if (*len == 0)
> +    {
> +      /* A zero-length Fortran string is "".  */
> +      char * tmp = internal_malloc_size (1);
> +      tmp[0] = '\0';
> +      *dest = tmp;
> +    }
> +  else

Do you zero-terminate the string because you don't want to allocate zero 
memory?  I don't know if allocating a pointer to zero memory would work, 
though.

- Tobi

Re: [libgfortran,patch] Zero-length strings messed up in library (PR33079)

[FX Coudert]

>> -  if (*len > 0)
>> +  if (*len == 0)
>> +    {
>> +      /* A zero-length Fortran string is "".  */
>> +      char * tmp = internal_malloc_size (1);
>> +      tmp[0] = '\0';
>> +      *dest = tmp;
>> +    }
>> +  else
>
> Do you zero-terminate the string because you don't want to allocate  
> zero memory?  I don't know if allocating a pointer to zero memory  
> would work, though.

I don't think you can portably call malloc(0) and expect a non-NULL  
pointer out of it. From http://www.opengroup.org/onlinepubs/009695399/ 
functions/malloc.html :
> If the size of the space requested is 0, the behavior is  
> implementation-defined: the value returned shall be either a null  
> pointer or a unique pointer.
Also, internal_malloc_size catches this case and returns NULL, so I  
think it's more reasonable to special-case zero right there. (And I  
do set the allocated memory to zero just to make sure we don't end up  
with unassigned memory.)

FX

Re: [libgfortran,patch] Zero-length strings messed up in library (PR33079)

[FX Coudert]

>> Do you zero-terminate the string because you don't want to  
>> allocate zero memory?  I don't know if allocating a pointer to  
>> zero memory would work, though.

I forgot to say: I'm waiting to hear back from you before committing,  
so if you still feel I'm not doing the right thing, please tell.

FX

Re: [libgfortran,patch] Zero-length strings messed up in library (PR33079)

[Tobias Schlüter]

FX Coudert wrote:
>>> -  if (*len > 0)
>>> +  if (*len == 0)
>>> +    {
>>> +      /* A zero-length Fortran string is "".  */
>>> +      char * tmp = internal_malloc_size (1);
>>> +      tmp[0] = '\0';
>>> +      *dest = tmp;
>>> +    }
>>> +  else
>>
>> Do you zero-terminate the string because you don't want to allocate 
>> zero memory?  I don't know if allocating a pointer to zero memory 
>> would work, though.
> 
> I don't think you can portably call malloc(0) and expect a non-NULL 
> pointer out of it. From 
> http://www.opengroup.org/onlinepubs/009695399/functions/malloc.html :
>> If the size of the space requested is 0, the behavior is 
>> implementation-defined: the value returned shall be either a null 
>> pointer or a unique pointer.
> Also, internal_malloc_size catches this case and returns NULL, so I 
> think it's more reasonable to special-case zero right there. (And I do 
> set the allocated memory to zero just to make sure we don't end up with 
> unassigned memory.)

An alternative would be to have a
  static char zero_length_string[0]; // Maybe [1]
at file scope and then do
  *dest = zero_length_string;
This would evade the unnecessary allocations.

Your patch is ok, if you don't like my suggestion better.

- Tobi

Re: [libgfortran,patch] Zero-length strings messed up in library (PR33079)

[Tobias Schlüter]

Tobias Schlüter wrote:
> An alternative would be to have a
>  static char zero_length_string[0]; // Maybe [1]
> at file scope and then do
>  *dest = zero_length_string;
> This would evade the unnecessary allocations.

Or the even shorter:
   static char zero_length_string;
   ...
   *dest = &zero_length_string;

- Tobi

Re: [libgfortran,patch] Zero-length strings messed up in library (PR33079)

[Tobias Burnus]

Tobias Schlüter wrote:
> Tobias Schlüter wrote:
>> An alternative would be to have a
>>  static char zero_length_string[0]; // Maybe [1]
>> at file scope and then do
>>  *dest = zero_length_string;
>> This would evade the unnecessary allocations.
> Or the even shorter:
>   static char zero_length_string;
>   *dest = &zero_length_string;
Indeed this would be a good option as we currently do not deallocate
that variable if len == 0.


    _gfortran_string_trim (&len.2, (void * *) &pstr.1, 1, &s[1]{lb: 1
sz: 1});
    bar (pstr.1, len.2);
    if (len.2 > 0)
      {
        {
          void * D.1379;

          D.1379 = (void *) pstr.1;
          if (D.1379 != 0B)
            {
              __builtin_free (D.1379);
            }

Tobias

Re: [libgfortran,patch] Zero-length strings messed up in library (PR33079)

[François-Xavier Coudert]

>>   static char zero_length_string;
>>   *dest = &zero_length_string;
> Indeed this would be a good option as we currently do not deallocate
> that variable if len == 0.

Committed the amended version as rev. 127584. Thanks to both of you
for reviewing this patch!

FX