date:20230805

Re: standardize and simplify GitHub submodule handling in ports?

2023-08-05 Thread Thomas Frohwein

On Sat, Aug 05, 2023 at 11:27:24PM +0200, Marc Espie wrote:
> Some comments already. I haven't looked very closely.

> On Sat, Aug 05, 2023 at 03:12:18PM -0400, Thomas Frohwein wrote:
> > The current draft hijacks post-extract target, but it would be easy to
> > add this to _post-extract-finalize in bsd.port.mk similar to how the
> > post-extract commands from modules are handled, if this is of interest.
> 
> Please do that.

Thanks, I updated the draft. Realized that including it with
MODULES=github is easiest and then this can just use
MODGITHUB_post-extract and doesn't need any custom code in bsd.port.mk.
I had a thinko in post-extract (needs to be '||', not '&&') which is
also corrected.

> > # where submodule distfiles will be stored
> > GHSM_DIST_SUBDIR ?= gh-submodules
> 
> Please keep to the GH_* subspace.
> 
> Already, modules usually use MOD* variable names, but in the GH case, that
> would be a bit long.

I renamed GHSM_* to GH_*. I wouldn't mind using MODGH_* if that's an
option, but MODGITHUB_* would be pretty unwieldy, especially if we were
to make the existing GH_{ACCOUNT,PROJECT,TAGNAME} etc. part of this.

[...]

> Please do a single loop.  That's slightly more readable for me.

yes, done.

[...]

> Also please draft a diff for port-modules(5)

I'm attaching a diff for port-modules.5, along with the updated
github.port.mk.

> I'm also wondering if keeping the main GH_* stuff in bsd.port.mk makes a lot
> of sense, instead of grouping everything in github.port.mk

I'm for it, maybe as a second step after the module for just the
submodule handling is done because there would be a lot of ports churn
with moving the main GH_* stuff out of bsd.port.mk.
# List of static dependencies. The format is:
# account project tag_or_commit target_dir # license
# Example:
# GH_SUBMODULES +=  moonlight-stream moonlight-common-c \
#   c9426a6a71c4162e65dde8c0c71a25f1dbca46ba \
#   third-party/moonlight-common-c # GPL-v3.0+
GH_SUBMODULES ?=

# Master site for github tarballs
GH_MASTER_SITES ?=  https://github.com/

# where submodule distfiles will be stored
GH_DIST_SUBDIR ?=   gh-submodules

# where submodules will be extracted to
GH_WRKSRC ?=${WRKSRC}

# Grab submodules by default with MASTER_SITES8. (Don't use 9 to avoid collision
# with language-specific mechanisms, like devel/cargo or lang/go.)
GH_MASTER_SITESN ?= 8
MASTER_SITES${GH_MASTER_SITESN} ?=  ${GH_MASTER_SITES}

# Default GitHub distfile suffix
GH_SUFX ?=  .tar.gz

.if defined(DISTNAME)
DISTFILES ?= ${DISTNAME}${EXTRACT_SUFX}
.elif !empty(GH_ACCOUNT) && !empty(GH_PROJECT)
DISTFILES ?= ${GH_DISTFILE}
.endif

# post-extract target for moving the submodules to GH_WRKSRC
MODGITHUB_post-extract = \
@${ECHO_MSG} "moving GitHub submodules to ${GH_WRKSRC}" ; \
mkdir -p ${GH_WRKSRC} ;

.for _ghaccount _ghproject _ghtagcommit _targetdir in ${GH_SUBMODULES}
DISTFILES +=
${GH_DIST_SUBDIR}/{}${_ghaccount}/${_ghproject}/archive/${_ghtagcommit}${GH_SUFX}:${GH_MASTER_SITESN}
MODGITHUB_post-extract += \
test -d ${GH_WRKSRC}/${_targetdir} && \
rm -rf ${GH_WRKSRC}/${_targetdir} ; \
mv ${WRKDIR}/${_ghproject}-${_ghtagcommit} ${GH_WRKSRC}/${_targetdir} ;
.endfor
Index: port-modules.5
===
RCS file: /cvs/src/share/man/man5/port-modules.5,v
retrieving revision 1.264
diff -u -p -r1.264 port-modules.5
--- port-modules.5  9 May 2023 19:44:06 -   1.264
+++ port-modules.5  6 Aug 2023 01:48:22 -
@@ -744,6 +744,15 @@ contains c++, this module provides
 .Ev MODGCC4_CPPLIBDEP
 and
 .Ev MODGCC4_CPPWANTLIB .
+.It github
+Set
+.Li GH_SUBMODULES += w x y z
+for each GitHub submodule to be used in the port, specifying GitHub account, 
project, commit hash, and the target directory relative to
+.Ev GH_WRKSRC
+.Po
+defaults to
+.Ev WRKSRC
+.Pc .
 .It gnu
 This module is documented in the main
 .Xr bsd.port.mk 5

hardclock(9), roundrobin: make roundrobin() an independent clock interrupt

2023-08-05 Thread Scott Cheloha

This is the next piece of the clock interrupt reorganization patch
series.

This patch removes the roundrobin() call from hardclock() and makes
roundrobin() an independent clock interrupt.

- Revise roundrobin() to make it a valid clock interrupt callback.
  It remains periodic.  It still runs at one tenth of the hardclock
  frequency.

- Account for multiple expirations in roundrobin().  If two or more
  intervals have elapsed we set SPCF_SHOULDYIELD immediately.

  This preserves existing behavior: hardclock() is called multiple
  times during clockintr_hardclock() if clock interrupts are blocked
  for long enough.

- Each schedstate_percpu has its own roundrobin() handle, spc_roundrobin.
  spc_roundrobin is established during sched_init_cpu(), staggered during
  the first clockintr_cpu_init() call, and advanced during clockintr_cpu_init().
  Expirations during suspend/resume are discarded.

- spc_rrticks and rrticks_init are now useless.  Delete them.

ok?

Also, yes, I see the growing pile of scheduler-controlled clock
interrupt handles.  My current plan is to move the setup code at the
end of clockintr_cpu_init() to a different routine, maybe something
like "sched_start_cpu()".  On the primary CPU you'd call it immediately
after cpu_initclocks().  On secondary CPUs you'd call it at the end of
cpu_hatch() just before cpu_switchto().

In any case, we will need to find a home for that code someplace.  It
can't stay in clockintr_cpu_init() forever.

Index: kern/sched_bsd.c
===
RCS file: /cvs/src/sys/kern/sched_bsd.c,v
retrieving revision 1.79
diff -u -p -r1.79 sched_bsd.c
--- kern/sched_bsd.c5 Aug 2023 20:07:55 -   1.79
+++ kern/sched_bsd.c5 Aug 2023 22:15:25 -
@@ -56,7 +56,6 @@
 
 
 intlbolt;  /* once a second sleep address */
-intrrticks_init;   /* # of hardclock ticks per roundrobin() */
 
 #ifdef MULTIPROCESSOR
 struct __mp_lock sched_lock;
@@ -69,21 +68,23 @@ uint32_tdecay_aftersleep(uint32_t, uin
  * Force switch among equal priority processes every 100ms.
  */
 void
-roundrobin(struct cpu_info *ci)
+roundrobin(struct clockintr *cl, void *cf)
 {
+   struct cpu_info *ci = curcpu();
struct schedstate_percpu *spc = >ci_schedstate;
+   uint64_t count;
 
-   spc->spc_rrticks = rrticks_init;
+   count = clockintr_advance(cl, hardclock_period * 10);
 
if (ci->ci_curproc != NULL) {
-   if (spc->spc_schedflags & SPCF_SEENRR) {
+   if (spc->spc_schedflags & SPCF_SEENRR || count >= 2) {
/*
 * The process has already been through a roundrobin
 * without switching and may be hogging the CPU.
 * Indicate that the process should yield.
 */
atomic_setbits_int(>spc_schedflags,
-   SPCF_SHOULDYIELD);
+   SPCF_SEENRR | SPCF_SHOULDYIELD);
} else {
atomic_setbits_int(>spc_schedflags,
SPCF_SEENRR);
@@ -695,8 +696,6 @@ scheduler_start(void)
 * its job.
 */
timeout_set(_to, schedcpu, _to);
-
-   rrticks_init = hz / 10;
schedcpu(_to);
 
 #ifndef SMALL_KERNEL
Index: kern/kern_sched.c
===
RCS file: /cvs/src/sys/kern/kern_sched.c,v
retrieving revision 1.84
diff -u -p -r1.84 kern_sched.c
--- kern/kern_sched.c   5 Aug 2023 20:07:55 -   1.84
+++ kern/kern_sched.c   5 Aug 2023 22:15:25 -
@@ -102,6 +102,12 @@ sched_init_cpu(struct cpu_info *ci)
if (spc->spc_profclock == NULL)
panic("%s: clockintr_establish profclock", __func__);
}
+   if (spc->spc_roundrobin == NULL) {
+   spc->spc_roundrobin = clockintr_establish(>ci_queue,
+   roundrobin);
+   if (spc->spc_roundrobin == NULL)
+   panic("%s: clockintr_establish roundrobin", __func__);
+   }
 
kthread_create_deferred(sched_kthreads_create, ci);
 
Index: kern/kern_clockintr.c
===
RCS file: /cvs/src/sys/kern/kern_clockintr.c,v
retrieving revision 1.30
diff -u -p -r1.30 kern_clockintr.c
--- kern/kern_clockintr.c   5 Aug 2023 20:07:55 -   1.30
+++ kern/kern_clockintr.c   5 Aug 2023 22:15:25 -
@@ -204,6 +204,11 @@ clockintr_cpu_init(const struct intrcloc
clockintr_stagger(spc->spc_profclock, profclock_period,
multiplier, MAXCPUS);
}
+   if (spc->spc_roundrobin->cl_expiration == 0) {
+   clockintr_stagger(spc->spc_roundrobin, hardclock_period,
+   multiplier, MAXCPUS);
+   }
+   clockintr_advance(spc->spc_roundrobin, hardclock_period * 10);
 
if

Re: standardize and simplify GitHub submodule handling in ports?

2023-08-05 Thread Marc Espie

Some comments already. I haven't looked very closely.

On Sat, Aug 05, 2023 at 03:12:18PM -0400, Thomas Frohwein wrote:
> The current draft hijacks post-extract target, but it would be easy to
> add this to _post-extract-finalize in bsd.port.mk similar to how the
> post-extract commands from modules are handled, if this is of interest.

Please do that.

> # where submodule distfiles will be stored
> GHSM_DIST_SUBDIR ?=   gh-submodules

Please keep to the GH_* subspace.

Already, modules usually use MOD* variable names, but in the GH case, that
would be a bit long.

> .for _ghaccount _ghproject _ghtagcommit _targetdir in ${GH_SUBMODULES}
> DISTFILES +=  
> ${GHSM_DIST_SUBDIR}/{}${_ghaccount}/${_ghproject}/archive/${_ghtagcommit}${GH_SUFX}:${GHSM_MASTER_SITESN}
> .endfor
> 
> # post-extract target for moving the submodules to the target directories
> GHSM_post-extract =
> .for _ghaccount _ghproject _ghtagcommit _targetdir in ${GH_SUBMODULES}
> GHSM_post-extract += \
>   test -d ${GHSM_WRKSR}/${_targetdir} || rm -rf 
> ${GHSM_WRKSRC}/${_targetdir} ; \
That line is weird.
>   mv ${WRKDIR}/${_ghproject}-${_ghtagcommit} ${GHSM_WRKSRC}/${_targetdir} 
> ;
> .endfor

Please do a single loop.  That's slightly more readable for me.

> # XXX: would best belong in _post-extract-finalize in bsd.port.mk rather than
> #  hijacking post-extract here
> post-extract:
>   @${ECHO_MSG} "moving GitHub submodules to ${GHSM_WRKSRC}" ;
>   mkdir -p ${GHSM_WRKSRC} ;
>   ${GHSM_post-extract}


Also please draft a diff for port-modules(5)


I'm also wondering if keeping the main GH_* stuff in bsd.port.mk makes a lot
of sense, instead of grouping everything in github.port.mk

standardize and simplify GitHub submodule handling in ports?

2023-08-05 Thread Thomas Frohwein

Hi,

GitHub projects using submodules seems to be a common enough case that
I'm wondering if it would be helpful to simplify and standardize it. I
got the idea when I saw how the modules for go and cargo pull in their
modules or crates, respectively.

The current state is that projects with submodules are either manually
packaged and hosted, or a combination of of MASTER_SITES0 through 9,
manual DISTFILES setting, and post-extract movement are used. What both
have in common is at least some maintenance burden and a significant
barrier for especially for those newer to the ports system.

The diff below proposes a module github.port.mk that aims to simplify
this process. At its core, the main way to manage submodules is to add
a line like the following:

GH_SUBMODULES+= luvit luv 093a977b82077591baefe1e880d37dfa2730bd54 \
luv # Apache-2.0

This way the GitHub tarball from the commit 093a977b from the project
luv by account luvit is added to the distfiles, and then the extracted
files are moved to ${GHSM_WRKSRC}/luv (GHSM_WRKSRC defaults to
${WRKSRC}). I saw license comments used with MODCARGO_CRATES and think
this would also be a good location for the submodule licenses.

This way, the multi-step setup and maintenance in MASTER_SITESX,
DISTFILES, and post-extract is reduced to essentially one location.

The current draft hijacks post-extract target, but it would be easy to
add this to _post-extract-finalize in bsd.port.mk similar to how the
post-extract commands from modules are handled, if this is of interest.

I'm attaching the github.port.mk file, as well as 3 diffs to show how
the simplified Makefiles look with this. A quick grep through the ports
tree shows there are at least a couple of dozen ports that could
benefit from a rework...
# List of static dependencies. The format is:
# account project tag_or_commit target_dir # license
# Example:
# GH_SUBMODULES +=  moonlight-stream moonlight-common-c \
#   c9426a6a71c4162e65dde8c0c71a25f1dbca46ba \
#   third-party/moonlight-common-c # GPL-v3.0+
GH_SUBMODULES ?=

# Master site for github tarballs
GH_MASTER_SITES ?=  https://github.com/

# where submodule distfiles will be stored
GHSM_DIST_SUBDIR ?= gh-submodules

# where submodules will be extracted to
GHSM_WRKSRC ?=  ${WRKSRC}

# Grab submodules by default with MASTER_SITES8. (Don't use 9 to avoid collision
# with language-specific mechanisms, like devel/cargo or lang/go.)
GHSM_MASTER_SITESN ?=   8
MASTER_SITES${GHSM_MASTER_SITESN} ?=${GH_MASTER_SITES}

# Default GitHub distfile suffix
GH_SUFX ?=  .tar.gz

.if defined(DISTNAME)
DISTFILES ?= ${DISTNAME}${EXTRACT_SUFX}
.elif !empty(GH_ACCOUNT) && !empty(GH_PROJECT)
DISTFILES ?= ${GH_DISTFILE}
.endif

.for _ghaccount _ghproject _ghtagcommit _targetdir in ${GH_SUBMODULES}
DISTFILES +=
${GHSM_DIST_SUBDIR}/{}${_ghaccount}/${_ghproject}/archive/${_ghtagcommit}${GH_SUFX}:${GHSM_MASTER_SITESN}
.endfor

# post-extract target for moving the submodules to the target directories
GHSM_post-extract =
.for _ghaccount _ghproject _ghtagcommit _targetdir in ${GH_SUBMODULES}
GHSM_post-extract += \
test -d ${GHSM_WRKSR}/${_targetdir} || rm -rf 
${GHSM_WRKSRC}/${_targetdir} ; \
mv ${WRKDIR}/${_ghproject}-${_ghtagcommit} ${GHSM_WRKSRC}/${_targetdir} 
;
.endfor

# XXX: would best belong in _post-extract-finalize in bsd.port.mk rather than
#  hijacking post-extract here
post-extract:
@${ECHO_MSG} "moving GitHub submodules to ${GHSM_WRKSRC}" ;
mkdir -p ${GHSM_WRKSRC} ;
${GHSM_post-extract}
Index: Makefile
===
RCS file: /cvs/ports/editors/neovim/Makefile,v
retrieving revision 1.37
diff -u -p -r1.37 Makefile
--- Makefile14 Jun 2023 07:47:57 -  1.37
+++ Makefile5 Aug 2023 19:06:09 -
@@ -23,22 +23,20 @@ CATEGORIES =editors devel
 HOMEPAGE = https://neovim.io
 MAINTAINER =   Edd Barrett 
 
+# Move static deps source code under WRKDIST so that they can be patched.
+STATIC_DEPS_WRKSRC =   ${WRKDIST}/static-deps
+GHSM_WRKSRC =  ${STATIC_DEPS_WRKSRC}
+
 # The versions listed here must match those in cmake.deps/CMakeLists.txt.
-LUV_VER =  093a977b82077591baefe1e880d37dfa2730bd54
-LUAJIT_VER =   505e2c03de35e2718eef0d2d3660712e06dadf1f
-LUACOMPAT_VER =v0.9
-
-MASTER_SITES0 =https://github.com/luvit/luv/archive/
-MASTER_SITES1 = https://github.com/LuaJIT/LuaJIT/archive/
-MASTER_SITES2 = https://github.com/keplerproject/lua-compat-5.3/archive/
-DISTFILES =${DISTNAME}${EXTRACT_SUFX} \
-   luv-{}${LUV_VER}${EXTRACT_SUFX}:0 \
-   luajit-{}${LUAJIT_VER}${EXTRACT_SUFX}:1 \
-   lua-compat-5.3-{}${LUACOMPAT_VER}${EXTRACT_SUFX}:2
-
-# Neovim: Apache 2.0 + Vim License
-# LuaJIT: MIT + public domain
-# libluv: Apache 2.0
+GH_SUBMODULES+=luvit luv 093a977b82077591baefe1e880d37dfa2730bd54 \
+   luv #

Re: [www] fix broken links on loongson.html

2023-08-05 Thread Moritz Buhl

Hi,

I noticed this today and commited your change.

mbuhl

On Tue, Jun 02, 2020 at 12:33:36PM +, Yifei Zhan wrote:
> Hi,
> 
> Several links on loongson.html are now broken as zkml.lemote.com is no
> longer being resolved.
> 
> I think it's a good idea to redirect them to their archived version on
> archive.org.
> 
> Cheers,
> Yifei Zhan
> 
> 

> Index: loongson.html
> ===
> RCS file: /cvs/www/loongson.html,v
> retrieving revision 1.66
> diff -u -p -r1.66 loongson.html
> --- loongson.html 19 May 2020 02:02:10 -  1.66
> +++ loongson.html 31 May 2020 06:55:56 -
> @@ -73,16 +73,16 @@ Specific hardware support is then writte
>  At the moment, the following machines are supported:
>  
>  
> - href="http://zkml.lemote.com/en/products/mini-computer/2010/0310/111.html;>Lemote
>  Fuloong 2F
> + href="https://web.archive.org/web/20190101154956/http://zkml.lemote.com/en/products/mini-computer/2010/0310/111.html;>Lemote
>  Fuloong 2F
>  mini-PC
>  
>  All on-board devices are supported, but the framebuffer is currently limited
>  to the 640x400x8 video mode set up by the firmware.
> - href="http://zkml.lemote.com/en/products/all-in-one/2010/0311/122.html;>Lemote
>  Lynloong all-in-one-PC
> + href="https://web.archive.org/web/20190101150011/http://zkml.lemote.com/en/products/all-in-one/2010/0311/122.html;>Lemote
>  Lynloong all-in-one-PC
>  
>  All on-board devices are supported, but the framebuffer is currently limited
>  to the 1360x768x16 video mode set up by the firmware.
> - href="http://zkml.lemote.com/en/products/Notebook/2010/0310/112.html;>Lemote 
> Yeeloong
> + href="https://web.archive.org/web/20160703160118/http://zkml.lemote.com/en/products/Notebook/2010/0310/112.html;>Lemote
>  Yeeloong
>  netbook
>  
>  Both the 8.9" and 10.1" models are supported.

Re: ldd: check read return value to avoid unitialized struct fields

2023-08-05 Thread Theo de Raadt

That looks better.


Lucas  wrote:

> Theo Buehler  wrote:
> > > - if (pread(fd, phdr, size, ehdr.e_phoff) != size) {
> > > + if ((nr = pread(fd, phdr, size, ehdr.e_phoff)) != -1) {
> > 
> > did you intend to check for == -1?
> > 
> > >   warn("read(%s)", name);
> > 
> > should that not say pread?
> 
> Indeed, thanks for spotting both things.
> 
> 
> ---
> commit 92f58b2a1cd576c3e72303004388ab1e9709e327 (ldd-read-rv)
> from: Lucas 
> date: Sat Aug  5 16:34:16 2023 UTC
>  
>  Check {,p}read return values consistently
>  
>  Check that read performs a full header read. Explicitly check against -1
>  for failure instead of < 0. Split pread error message between error
>  handling and short reads. Promote size from int to size_t.
>  
>  M  libexec/ld.so/ldd/ldd.c
> 
> diff 194ff02fb6be247e27fe964e901d891d99ec0b74 
> 92f58b2a1cd576c3e72303004388ab1e9709e327
> commit - 194ff02fb6be247e27fe964e901d891d99ec0b74
> commit + 92f58b2a1cd576c3e72303004388ab1e9709e327
> blob - 9e8c5065cd843ff36d91efcb868b94ffd4c98365
> blob + 532feb9855a03480c289fa2188768657a4f82e7c
> --- libexec/ld.so/ldd/ldd.c
> +++ libexec/ld.so/ldd/ldd.c
> @@ -96,7 +96,9 @@ doit(char *name)
>  {
>   Elf_Ehdr ehdr;
>   Elf_Phdr *phdr;
> - int fd, i, size, status, interp=0;
> + size_t size;
> + ssize_t nr;
> + int fd, i, status, interp=0;
>   char buf[PATH_MAX];
>   struct stat st;
>   void * dlhandle;
> @@ -118,11 +120,16 @@ doit(char *name)
>   return 1;
>   }
>  
> - if (read(fd, , sizeof(ehdr)) < 0) {
> + if ((nr = read(fd, , sizeof(ehdr))) == -1) {
>   warn("read(%s)", name);
>   close(fd);
>   return 1;
>   }
> + if (nr != sizeof(ehdr)) {
> + warnx("%s: incomplete ELF header", name);
> + close(fd);
> + return 1;
> + }
>  
>   if (!IS_ELF(ehdr) || ehdr.e_machine != ELF_TARG_MACH) {
>   warnx("%s: not an ELF executable", name);
> @@ -140,12 +147,18 @@ doit(char *name)
>   err(1, "reallocarray");
>   size = ehdr.e_phnum * sizeof(Elf_Phdr);
>  
> - if (pread(fd, phdr, size, ehdr.e_phoff) != size) {
> - warn("read(%s)", name);
> + if ((nr = pread(fd, phdr, size, ehdr.e_phoff)) == -1) {
> + warn("pread(%s)", name);
>   close(fd);
>   free(phdr);
>   return 1;
>   }
> + if (nr != size) {
> + warnx("%s: incomplete program header", name);
> + close(fd);
> + free(phdr);
> + return 1;
> + }
>   close(fd);
>  
>   for (i = 0; i < ehdr.e_phnum; i++)
>

Re: ldd: check read return value to avoid unitialized struct fields

2023-08-05 Thread Lucas

Theo Buehler  wrote:
> > -   if (pread(fd, phdr, size, ehdr.e_phoff) != size) {
> > +   if ((nr = pread(fd, phdr, size, ehdr.e_phoff)) != -1) {
> 
> did you intend to check for == -1?
> 
> > warn("read(%s)", name);
> 
> should that not say pread?

Indeed, thanks for spotting both things.


---
commit 92f58b2a1cd576c3e72303004388ab1e9709e327 (ldd-read-rv)
from: Lucas 
date: Sat Aug  5 16:34:16 2023 UTC
 
 Check {,p}read return values consistently
 
 Check that read performs a full header read. Explicitly check against -1
 for failure instead of < 0. Split pread error message between error
 handling and short reads. Promote size from int to size_t.
 
 M  libexec/ld.so/ldd/ldd.c

diff 194ff02fb6be247e27fe964e901d891d99ec0b74 
92f58b2a1cd576c3e72303004388ab1e9709e327
commit - 194ff02fb6be247e27fe964e901d891d99ec0b74
commit + 92f58b2a1cd576c3e72303004388ab1e9709e327
blob - 9e8c5065cd843ff36d91efcb868b94ffd4c98365
blob + 532feb9855a03480c289fa2188768657a4f82e7c
--- libexec/ld.so/ldd/ldd.c
+++ libexec/ld.so/ldd/ldd.c
@@ -96,7 +96,9 @@ doit(char *name)
 {
Elf_Ehdr ehdr;
Elf_Phdr *phdr;
-   int fd, i, size, status, interp=0;
+   size_t size;
+   ssize_t nr;
+   int fd, i, status, interp=0;
char buf[PATH_MAX];
struct stat st;
void * dlhandle;
@@ -118,11 +120,16 @@ doit(char *name)
return 1;
}
 
-   if (read(fd, , sizeof(ehdr)) < 0) {
+   if ((nr = read(fd, , sizeof(ehdr))) == -1) {
warn("read(%s)", name);
close(fd);
return 1;
}
+   if (nr != sizeof(ehdr)) {
+   warnx("%s: incomplete ELF header", name);
+   close(fd);
+   return 1;
+   }
 
if (!IS_ELF(ehdr) || ehdr.e_machine != ELF_TARG_MACH) {
warnx("%s: not an ELF executable", name);
@@ -140,12 +147,18 @@ doit(char *name)
err(1, "reallocarray");
size = ehdr.e_phnum * sizeof(Elf_Phdr);
 
-   if (pread(fd, phdr, size, ehdr.e_phoff) != size) {
-   warn("read(%s)", name);
+   if ((nr = pread(fd, phdr, size, ehdr.e_phoff)) == -1) {
+   warn("pread(%s)", name);
close(fd);
free(phdr);
return 1;
}
+   if (nr != size) {
+   warnx("%s: incomplete program header", name);
+   close(fd);
+   free(phdr);
+   return 1;
+   }
close(fd);
 
for (i = 0; i < ehdr.e_phnum; i++)

Re: ldd: check read return value to avoid unitialized struct fields

2023-08-05 Thread Theo Buehler

> - if (pread(fd, phdr, size, ehdr.e_phoff) != size) {
> + if ((nr = pread(fd, phdr, size, ehdr.e_phoff)) != -1) {

did you intend to check for == -1?

>   warn("read(%s)", name);

should that not say pread?

Re: ldd: check read return value to avoid unitialized struct fields

2023-08-05 Thread Lucas

"Theo de Raadt"  wrote:
> Nope, that is not correct.
> 
> errno is not being cleared.  It just happens to be zero.  Future
> code changes could insert another operation above which would set
> errno, and then this would print a report about that error.

Although I was being sarcastic with """Everything is alright""", yes,
correct. Point taken.

> No, your diff is still wrong.
> 
> errno is only updated when a system call returns -1.
> 
> So your diff is looking at an old, unrelated, errno.

How? This is now correctly looking at errno only when {,p}read returns
-1, and is using warnx in the other cases.

Re: ldd: check read return value to avoid unitialized struct fields

2023-08-05 Thread Theo de Raadt

Lucas  wrote:

> "Theo de Raadt"  wrote:
> > What errno is being printed here?
> 
> """Everything is alright""" error,
> 
>   $ : >empty && ./obj/ldd empty
>   ldd: read(empty): Undefined error: 0
> 
> which would be the same as a short read in the pread below.

Nope, that is not correct.

errno is not being cleared.  It just happens to be zero.  Future
code changes could insert another operation above which would set
errno, and then this would print a report about that error.

No, your diff is still wrong.

errno is only updated when a system call returns -1.

So your diff is looking at an old, unrelated, errno.

And instead of the previous diff which did this once, you are now
doing it 5 times.

> ---
> commit f1255c0035aa37752a298b752fd20215a1d7adef (ldd-read-rv)
> from: Lucas 
> date: Sat Aug  5 14:36:58 2023 UTC
>  
>  Check {,p}read return values consistently
>  
>  Check that read performs a full header read. Explicitly check against -1
>  for failure instead of < 0. Split pread error message between error
>  handling and short reads. Promote size from int to size_t.
>  
>  M  libexec/ld.so/ldd/ldd.c
> 
> diff 7b0c383483702d9a26856c2b4754abb44950ed82 
> f1255c0035aa37752a298b752fd20215a1d7adef
> commit - 7b0c383483702d9a26856c2b4754abb44950ed82
> commit + f1255c0035aa37752a298b752fd20215a1d7adef
> blob - 9e8c5065cd843ff36d91efcb868b94ffd4c98365
> blob + 12777f2420a6a74f9f456f080c207bf47760b258
> --- libexec/ld.so/ldd/ldd.c
> +++ libexec/ld.so/ldd/ldd.c
> @@ -96,7 +96,9 @@ doit(char *name)
>  {
>   Elf_Ehdr ehdr;
>   Elf_Phdr *phdr;
> - int fd, i, size, status, interp=0;
> + size_t size;
> + ssize_t nr;
> + int fd, i, status, interp=0;
>   char buf[PATH_MAX];
>   struct stat st;
>   void * dlhandle;
> @@ -118,11 +120,16 @@ doit(char *name)
>   return 1;
>   }
>  
> - if (read(fd, , sizeof(ehdr)) < 0) {
> + if ((nr = read(fd, , sizeof(ehdr))) == -1) {
>   warn("read(%s)", name);
>   close(fd);
>   return 1;
>   }
> + if (nr != sizeof(ehdr)) {
> + warnx("%s: incomplete ELF header", name);
> + close(fd);
> + return 1;
> + }
>  
>   if (!IS_ELF(ehdr) || ehdr.e_machine != ELF_TARG_MACH) {
>   warnx("%s: not an ELF executable", name);
> @@ -140,12 +147,18 @@ doit(char *name)
>   err(1, "reallocarray");
>   size = ehdr.e_phnum * sizeof(Elf_Phdr);
>  
> - if (pread(fd, phdr, size, ehdr.e_phoff) != size) {
> + if ((nr = pread(fd, phdr, size, ehdr.e_phoff)) != -1) {
>   warn("read(%s)", name);
>   close(fd);
>   free(phdr);
>   return 1;
>   }
> + if (nr != size) {
> + warnx("%s: incomplete program header", name);
> + close(fd);
> + free(phdr);
> + return 1;
> + }
>   close(fd);
>  
>   for (i = 0; i < ehdr.e_phnum; i++)
>

Re: ldd: check read return value to avoid unitialized struct fields

2023-08-05 Thread Lucas

"Theo de Raadt"  wrote:
> What errno is being printed here?

"""Everything is alright""" error,

$ : >empty && ./obj/ldd empty
ldd: read(empty): Undefined error: 0

which would be the same as a short read in the pread below.

Bigger suggestion below, addressing both read and pread. Also promoted
size to a size_t, as the multiplication could overflow an int. read < 0
was changed into read == -1 for alignment with read(2).

There are an open and wait that could receive the same < 0 vs == -1
treatment.

Bike can be repainted at will.

-Lucas


---
commit f1255c0035aa37752a298b752fd20215a1d7adef (ldd-read-rv)
from: Lucas 
date: Sat Aug  5 14:36:58 2023 UTC
 
 Check {,p}read return values consistently
 
 Check that read performs a full header read. Explicitly check against -1
 for failure instead of < 0. Split pread error message between error
 handling and short reads. Promote size from int to size_t.
 
 M  libexec/ld.so/ldd/ldd.c

diff 7b0c383483702d9a26856c2b4754abb44950ed82 
f1255c0035aa37752a298b752fd20215a1d7adef
commit - 7b0c383483702d9a26856c2b4754abb44950ed82
commit + f1255c0035aa37752a298b752fd20215a1d7adef
blob - 9e8c5065cd843ff36d91efcb868b94ffd4c98365
blob + 12777f2420a6a74f9f456f080c207bf47760b258
--- libexec/ld.so/ldd/ldd.c
+++ libexec/ld.so/ldd/ldd.c
@@ -96,7 +96,9 @@ doit(char *name)
 {
Elf_Ehdr ehdr;
Elf_Phdr *phdr;
-   int fd, i, size, status, interp=0;
+   size_t size;
+   ssize_t nr;
+   int fd, i, status, interp=0;
char buf[PATH_MAX];
struct stat st;
void * dlhandle;
@@ -118,11 +120,16 @@ doit(char *name)
return 1;
}
 
-   if (read(fd, , sizeof(ehdr)) < 0) {
+   if ((nr = read(fd, , sizeof(ehdr))) == -1) {
warn("read(%s)", name);
close(fd);
return 1;
}
+   if (nr != sizeof(ehdr)) {
+   warnx("%s: incomplete ELF header", name);
+   close(fd);
+   return 1;
+   }
 
if (!IS_ELF(ehdr) || ehdr.e_machine != ELF_TARG_MACH) {
warnx("%s: not an ELF executable", name);
@@ -140,12 +147,18 @@ doit(char *name)
err(1, "reallocarray");
size = ehdr.e_phnum * sizeof(Elf_Phdr);
 
-   if (pread(fd, phdr, size, ehdr.e_phoff) != size) {
+   if ((nr = pread(fd, phdr, size, ehdr.e_phoff)) != -1) {
warn("read(%s)", name);
close(fd);
free(phdr);
return 1;
}
+   if (nr != size) {
+   warnx("%s: incomplete program header", name);
+   close(fd);
+   free(phdr);
+   return 1;
+   }
close(fd);
 
for (i = 0; i < ehdr.e_phnum; i++)

Re: standardize and simplify GitHub submodule handling in ports?

hardclock(9), roundrobin: make roundrobin() an independent clock interrupt

Re: standardize and simplify GitHub submodule handling in ports?

standardize and simplify GitHub submodule handling in ports?

Re: [www] fix broken links on loongson.html

Re: ldd: check read return value to avoid unitialized struct fields

Re: ldd: check read return value to avoid unitialized struct fields

Re: ldd: check read return value to avoid unitialized struct fields

Re: ldd: check read return value to avoid unitialized struct fields

Re: ldd: check read return value to avoid unitialized struct fields

Re: ldd: check read return value to avoid unitialized struct fields

11 matches

Site Navigation

Mail list logo

Footer information