Bug 257599 - [exp-run] ar/ranlib error handling fix
Summary: [exp-run] ar/ranlib error handling fix
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Package Infrastructure (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Port Management Team
URL: https://reviews.freebsd.org/D31402
Keywords:
Depends on: 257744
Blocks:
  Show dependency treegraph
 
Reported: 2021-08-03 18:44 UTC by Ed Maste
Modified: 2021-08-10 21:19 UTC (History)
3 users (show)

See Also:


Attachments
Add error handling to ar/ranlib (63.99 KB, patch)
2021-08-03 18:44 UTC, Ed Maste
no flags Details | Diff
Updated ar exit status patch with fixes from markj (13.21 KB, patch)
2021-08-04 14:37 UTC, Ed Maste
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ed Maste freebsd_committer freebsd_triage 2021-08-03 18:44:17 UTC
Created attachment 226923 [details]
Add error handling to ar/ranlib

Currently FreeBSD's ar / ranlib does not return an error exit status in the case of a missing file, etc. (unlike GNU ar, ELF Tool Chain ar, or llvm-ar). I would go ahead without an exp-run, but HardenedBSD IRC folks (from where I learned about this) suggested some ports may have come to rely on this behaviour.
Comment 1 Shawn Webb 2021-08-03 20:54:59 UTC
(In reply to Ed Maste from comment #0)
One example port is math/lapacke. There's more than just that one, though I can't remember the other ports.
Comment 2 Ed Maste freebsd_committer freebsd_triage 2021-08-04 14:37:22 UTC
Created attachment 226945 [details]
Updated ar exit status patch with fixes from markj

Change is now reviewed (with a few updates), patch updated. I will commit once the exp-run is complete if there is no significant fallout.
Comment 3 Antoine Brodin freebsd_committer freebsd_triage 2021-08-04 15:20:27 UTC
(In reply to Ed Maste from comment #2)
Exp-run already started with the previous version of the patch,  is this an issue?
Comment 4 Ed Maste freebsd_committer freebsd_triage 2021-08-04 15:40:40 UTC
(In reply to Antoine Brodin from comment #3)
I think the previous version should be fine, at least for the gross error that prompted this case originally. The original issue was observed with `ranlib does_not_exist`, which had exit status 0. Incorrect invocation of ar / ranlib will return exit status 1 with the first patch.

One issue Mark caught was that the original version of the patch did not check the return value from the individual ar d/m/p/q/r/t/x operation, so errors relating to corrupted archives or object files might have been missed. I think this is a rather unlikely case.
Comment 5 Antoine Brodin freebsd_committer freebsd_triage 2021-08-10 08:31:38 UTC
New failure log:

http://gohan04.nyi.freebsd.org/data/main-amd64-PR257599-default/2021-08-08_12h01m59s/logs/errors/lapacke-3.10.0.log

44 ports were skipped due to this failure
Comment 6 commit-hook freebsd_committer freebsd_triage 2021-08-10 21:07:15 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=6fd5f57d0613a9f6d1816a912f21728b0d12435a

commit 6fd5f57d0613a9f6d1816a912f21728b0d12435a
Author:     Ed Maste <emaste@FreeBSD.org>
AuthorDate: 2021-08-10 16:01:23 +0000
Commit:     Ed Maste <emaste@FreeBSD.org>
CommitDate: 2021-08-10 21:03:05 +0000

    math/lapack: remove superfluous and broken ranlib invocation

    FreeBSD's base system ar and ranlib have a bug where they exit with
    status 0 (success) even in case of fatal errors.  This hid the fact that
    math/lapacke was invoking ranlib on a non-existent file.  (Presumably the
    ranlib invocation was correct when introduced, but broken by some
    rework of lapack's upstream build system).

    Use of ranlib is generally unncessary, assuming the -s flag is passed to
    ar (as is typical, and as done here), so just delete the invocation.

    See PR 257599 and review D31402 for the ar/ranlib base system bug.

    PR:             257599, 257744
    Reviewed by:    jrm
    Tested by:      jrm
    Approved by:    kevans (ports), portmgr (implicit, blanket: build fix)
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D31492

 math/lapack/files/static.mk | 1 -
 1 file changed, 1 deletion(-)
Comment 7 Ed Maste freebsd_committer freebsd_triage 2021-08-10 21:16:21 UTC
commit 38911b3c2c7dbb9a097b44856472ebbbedde71fc
Author: Ed Maste <emaste@FreeBSD.org>
Date:   Tue Aug 3 14:30:06 2021 -0400

    ar: provide error exit status upon failure
    
    Previously ar and ranlib returned with exit status 0 (success) in the
    case of a missing file or other error.  Update to use error handling
    similar to that added by ELF Tool Chain after that project forked
    FreeBSD's ar.
    
    PR:             PR257599 [exp-run]
    Reported by:    Shawn Webb, gehmehgeh (on HardenedBSD IRC)
    Reviewed by:    markj
    Obtained from:  elftoolchain
    MFC after:      2 months
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D31402
Comment 8 Ed Maste freebsd_committer freebsd_triage 2021-08-10 21:19:00 UTC
(In reply to Antoine Brodin from comment #5)
Thanks for the exp-run Antoine,

> 44 ports were skipped due to this failure

IMO the likelihood of one of these ports having the same issue is quite low so I committed the base system change. If there's any other fallout I'll help address it after the fact.