bugGNU roff - Bugs: bug #62398, [troff] input.cpp: argument...

 
 

bug #62398: [troff] input.cpp: argument "len" is zero in "new unsigned char[len]"

Submitter:  Bjarni Ingi Gislason <bjarniig>
Submitted:  Tue 03 May 2022 12:54:07 AM UTC
   
 
Category:  Core Severity:  2 - Minor
Item Group:  Lint Status:  Fixed
Privacy:  Public Assigned to:  gbranden
Open/Closed:  Closed Planned Release:  1.24.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 10 Jul 2023 08:44:14 AM UTC, comment #11: 


commit 5a80c2412b89f64a31236ab2ee5719a94a6b01e3
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Apr 3 19:30:56 2023 -0500

    [troff]: Skip allocation of zero-length arrays.

    * src/roff/troff/input.cpp (temp_iterator::temp_iterator): Skip
      allocation of zero-length arrays.  Resolves "-Walloc-zero" warning
      from GCC.

    Fixes <https://savannah.gnu.org/bugs/?62398>.  Thanks to Bjarni Ingi
    Gislason for the report.

    It is not necessary to make conditional the subsequent `delete[]` of a
    null pointer.  "If the _delete-expression_ calls the implementation
    deallocation function (3.7.3.2), and if the operand of the delete
    expression is not the null pointer constant, the deallocation function
    will deallocate the storage referenced by the pointer thus rendering the
    pointer invalid" (ISO/IEC 14882-1998, §5.3.5, paragraph 4).  Or as
    Stroustrup puts it, "Applying _delete_ to zero has no effect." (_The C++
    Programming Language, Special Edition_, p. 128).

    Also annotate some null pointers with `nullptr` comments to ease any
    future transition to C++11, which defines it as a keyword.


G. Branden Robinson <gbranden>
Group administrator
Fri 07 Apr 2023 04:45:08 PM UTC, comment #10: 


commit c09128d9ef8c8181e61614ae762308d2ac27b29f
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Mon Apr 3 19:30:56 2023 -0500

    [troff]: Skip allocation of zero-length arrays.

    * src/roff/troff/input.cpp (temp_iterator::temp_iterator): Skip
      allocation of zero-length arrays.  Resolves "-Walloc-zero" warning
      from GCC.

    Fixes <https://savannah.gnu.org/bugs/?62398>.  Thanks to Bjarni Ingi
    Gislason for the report.

    It is not necessary to make conditional the subsequent `delete[]` of a
    null pointer.  "If the _delete-expression_ calls the implementation
    deallocation function (3.7.3.2), and if the operand of the delete
    expression is not null pointer constant, the deallocation function will
    deallocate the storage referenced by the pointer thus rendering the
    pointer invalid" (ISO/IEC 14882-1998, §5.3.5, paragraph 4).  Or as
    Stroustrup puts it, "Applying _delete_ to zero has no effect." (_The C++
    Programming Language, Special Edition_, p. 128).


G. Branden Robinson <gbranden>
Group administrator
Sun 09 Oct 2022 09:09:22 AM UTC, comment #9: 

  Also see the original report, bug #47160 (closed as fixed).

Bjarni Ingi Gislason <bjarniig>
Sat 08 Oct 2022 06:09:45 PM UTC, comment #8: 

In the original code, I also wonder--since the "new" isn't protected by a "len > 0" check--why the memcpy is.  Surely a memcpy() on a zero-length string is effectively a no-op.

Dave <barx>
Group Member
Mon 30 May 2022 07:41:00 PM UTC, comment #7: 

  See also bug #62547.

Bjarni Ingi Gislason <bjarniig>
Fri 27 May 2022 11:56:00 PM UTC, comment #6: 

The gcc documentation itself, concerning this warning (http://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Walloc-zero), claims "the behavior of these functions when called with a zero size differs among implementations."

Dave <barx>
Group Member
Mon 23 May 2022 12:51:50 AM UTC, comment #5: 


comment #4:

> That tells us what one compiler on one platform does, which is of limited usefulness for a package that needs to compile in multiple environments.
>
> If the C++ standard says this is the defined behavior, then the proposed replacement code is functionally equivalent to the existing code.
>
> If the standard leaves this behavior undefined, then there's a problem with the existing code.
>
> If the standard defines the behavior but some compiler is known to be noncompliant, that's a different problem--arguably not ours.


I would also add that having a null pointer is not itself a problem in C or C++--as long as it is not dereferenced.  Such pointers are commonly used, in what Tony Hoare called his "billion dollar mistake" https://www.infoq.com/presentations/Null-References-The-Billion-Dollar-Mistake-Tony-Hoare/.

There may be a code hygiene issue here but unless it rises to the level of causing operational problems in groff, the ticket severity must remain "minor".

If a contributor would like to audit the code for problems like this, prepare patches, and regression-test them, that would be welcome.

G. Branden Robinson <gbranden>
Group administrator
Mon 23 May 2022 12:36:07 AM UTC, comment #4: 

comment #2:

> It sounds like this non-default compiler warning does not
> indicate the presence of incorrect program operation.


My question in comment #1 was genuine--I don't know what C++ guarantees in this situation.

comment #3:

> The "new" operator (with len = 0) creates a pointer that
> points to a "NULL" (nullptr) in the memory.
>
> This can be seen when output is inserted to the code, for example


That tells us what one compiler on one platform does, which is of limited usefulness for a package that needs to compile in multiple environments.

If the C++ standard says this is the defined behavior, then the proposed replacement code is functionally equivalent to the existing code.

If the standard leaves this behavior undefined, then there's a problem with the existing code.

If the standard defines the behavior but some compiler is known to be noncompliant, that's a different problem--arguably not ours.

Dave <barx>
Group Member
Sun 22 May 2022 03:32:01 PM UTC, comment #3: 

  The "new" operator (with len = 0) creates a pointer that points to a
"NULL" (nullptr) in the memory.

  This can be seen when output is inserted to the code, for example

if (len = 0) {
    fprintf(stderr, "%s %s:%d allocated size = %d, base = %p; string = %s\n",
            "warning:", _FILE_, _LINE_, len, base, s);
}

  Thus this is a superfluous creation of pointers.

Bjarni Ingi Gislason <bjarniig>
Thu 19 May 2022 09:44:10 AM UTC, comment #2: 

It sounds like this non-default compiler warning does not indicate the presence of incorrect program operation.

Updating Item Group and Severity accordingly.

G. Branden Robinson <gbranden>
Group administrator
Thu 05 May 2022 03:54:44 PM UTC, comment #1: 

Does C++ have defined behavior when asked to allocate a zero-length array?  If it does, the warning seems spurious and should probably be ignored (or disabled, which appears to be the default anyway).

Dave <barx>
Group Member
Tue 03 May 2022 12:54:07 AM UTC, original submission:  

Subject: .../troff/input.cpp: argument "len" is zero in "new unsigned char[len]"

  Output from gcc with option -Walloc-zero:

[...]
  CXX      src/roff/troff/input.o
In constructor 'temp_iterator::temp_iterator(const char*, int)',
    inlined from 'input_iterator* make_temp_iterator(const char*)' at ../src/roff/troff/input.cpp:3627:34:
../src/roff/troff/input.cpp:3611:31: warning: argument 1 value is zero [-Walloc-zero]
 3611 |   base = new unsigned char[len];
      |                               ^
In file included from /usr/include/c++/11/bits/stl_iterator.h:82,
                 from /usr/include/c++/11/bits/stl_algobase.h:67,
                 from /usr/include/c++/11/vector:60,
                 from ../src/roff/troff/charinfo.h:20,
                 from ../src/roff/troff/input.cpp:31:
/usr/include/c++/11/new: In function 'input_iterator* make_temp_iterator(const char*)':
/usr/include/c++/11/new:128:26: note: in a call to allocation function 'void* operator new [](std::size_t)' declared here
  128 | _GLIBCXX_NODISCARD void* operator new[](std::size_t) _GLIBCXX_THROW (std::bad_alloc)
      |                          ^~~~~~~~
  CXX      src/roff/troff/mtsm.o
[...]

  The code is:

  base = new unsigned char[len];
  if (len > 0)
    memcpy(base, s, len);
  ptr = base;
  eptr = base + len;

IS THIS BETTER ?

// gcc -Walloc-zero warns when len = 0

  if (len > 0) {
    base = new unsigned char[len];
    memcpy(base, s, len);
    ptr = base;
    eptr = base + len;
  } else {
    base = 0;
    ptr = 0;
    eptr = 0;
  }

N.B.  No warning


Bjarni Ingi Gislason <bjarniig>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by gbranden (Posted a comment)
  • -email is unavailable- added by barx (Posted a comment)
  • -email is unavailable- added by bjarniig (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-07-10 gbranden StatusReady for Merge Fixed
        Open/ClosedOpen Closed
        Planned ReleaseNone 1.24.0
        Summary.../troff/input.cpp: argument &quot;len&quot; is zero in &quot;new unsigned char[len]&quot; [troff] input.cpp: argument "len" is zero in "new unsigned char[len]"
    2023-04-07 gbranden StatusNone Ready for Merge
        Assigned toNone gbranden
    2022-05-19 gbranden Severity3 - Normal 2 - Minor
        Item GroupWarning/Suspicious behaviour Lint

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code