[MIR] iniparser (dependency of mtd-utils)

ddstreet: if you need any further info or if anything is missing please let me know. Per the MIR IRC meeting it looks like there is a partial concern that the library may be redundant, albeit with a different API. Any further feedback?

mtd-utils is just here for tracking, it needs no review.
iniparser does need one and ddstreet is already assigned.

[Summary]
While there is an existing package in main that provides a C INI
parsing library, the mtd-utils package (already in main since at
least bionic) has simply embedded this library (and it continues
to do so). The only change was a patch added to mtd-utils late
last year to link to the system libiniparser.so instead of using
the embedded iniparser code. So the duplication seems necessary,
as changing mtdutils over to use a different ini parser library
would be a significant unnecessary amount of work (and unlikely
for upstream mtd-utils to accept).

There are some concerns listed under notes, but none required.
So this is ACK from MIR team.

This does need a security review, so I'll assign ubuntu-security

List of specific binary packages to be promoted to main:
- libiniparser1

Notes:
I noted 2 additional concerns, but considering the fact that this
library code has been embedded in the mtd-utils package, in main,
since bionic, I don't believe either should block MIR.

Recommended TODOs:
- The build should run 'make check', or otherwise run the tests
  in the test/ directory; a failure in those tests (or in the
  CMake test) should fail the build
  Note: opened LP:#1915866 for this
- The apparent slow/stopped pace of updates upstream is concerning;
  Debian and/or Ubuntu should pull any upstream commits not currently
  included, as well as reviewing upstream bugs for possible patching
  in Debian/Ubuntu

[Duplication]
- existing main package 'libini-config5'
  provided by source package 'ding-libs'

[Dependencies]
OK:
- no other Dependencies to MIR due to this

Problems:
- -dev and -doc packages that need exclusion

[Embedded sources and static linking]
OK:
- no embedded source present
- no static linking (but does provide static libs)

[Security]
OK:
- history of CVEs does not look concerning (no CVEs)
- does not run a daemon as root
- does not use webkit1,2
- does not use lib*v8 directly
- does not open a port
- does not process arbitrary web content
- does not use centralized online accounts
- does not integrate arbitrary javascript into the desktop
- does not deal with system authentication (eg, pam), etc)

Problems:
- does not parse data formats, but does parse INI (config) format
- upstream has some open bugs possibly security-related

[Common blockers]
OK:
- does not FTBFS currently
- The package has a team bug subscriber (foundations)
- no translation present, but none needed for this case (not user visible)
- not a python/go package, no extra constraints to consider in that regard

Problems:
- has 2 test suites, but only runs 1 at build time
  - runs CMake test, but does not run test suite in test/ dir

[Packaging red flags]
OK:
- Ubuntu does not carry a delta
- symbols tracking is in place
- d/watch is present and looks ok
- the current release is packaged
- promoting this does not seem to cause issues for MOTUs that so far
  maintained the package
- no massive Lintian warnings
- d/rules is rather clean
- Does not have Built-Using
- not go package

Problems:
- Upstream update history is slow/stopped
- Debian/Ubuntu update history is slow, but hard to determine because last upstream release was 2017

[Upstream red flags]
OK:
-...

Patch attached to LP:#1915866 to address issue with package tests.
I'm holding off on an attempt to merge that patch on LP:#1915866 just yet as I expect that the security team will have suggestions that we will want to implement.

I reviewed iniparser 4.1-4 as checked into hirsute. This shouldn't be
considered a full audit but rather a quick gauge of maintainability.

iniparser is a small C library for parsing ini-style configuration files.

- CVE History:
  - None, however in 2016 a security issue was raised on their Github
    https://github.com/ndevilla/iniparser/issues/68 - it took a few months
    for this to be fixed but at worst this was a DoS issue - no CVE was
    ever assigned.
- No security relevant Build-Depends
- No pre/post inst/rm scripts
- No init scripts
- No systemd units
- No dbus services
- No setuid binaries
- No binaries in PATH
- No sudo fragments
- No polkit files
- No udev rules
- unit tests / autopkgtests
  - autopkgtests run a couple of the examples shipped in the package source
  - unit tests are run during the build and these look reasonably
    extensive - there was one issue discovered by sbeattie via Coverity
    which prevented one of the tests cases in one of the tests from running
    https://github.com/ndevilla/iniparser/issues/131
- No cron jobs
- Build logs:
  - ERRORS / WARNINGS
    - During build gcc outputs the following warning:
      src/iniparser.c: In function ‘iniparser_load’:
      src/iniparser.c:791:32: warning: ‘__builtin___sprintf_chk’ may write a terminating nul past the end of the destination [-Wformat-overflow=]
    - This happens at the following code:

      sprintf(tmp, "%s:%s", section, key);

      In this case, where tmp, section and key are declared as:

      char section [ASCIILINESZ+1] ;
      char key [ASCIILINESZ+1] ;
      char tmp [(ASCIILINESZ * 2) + 1] ;

      As such, at most section and key are both ASCIILINESZ plus 1 colon
      separator fills then entire tmp buffer and leaves no space for a
      terminating NUL - so this looks like a real bug which could result in
      a 1-byte stack buffer overflow. This has already been fixed upstream
      in
      https://github.com/ndevilla/iniparser/commit/2412f165bcfde4ad8e3426fd59f2a920492b8c19
      so this patch should be integrated into our package.

  - LINTIAN FAILURES
    - Only 2 relevant lintian warnings:

      W: iniparser source: uses-deprecated-adttmp debian/tests/iniexample (line 4)
      W: iniparser source: uses-deprecated-adttmp debian/tests/parse (line 4)

- No processes spawned
- Memory management
  - Appears to be careful for the main library code (there are a bunch of
    issues in the unit test code but in general they are minor anyway)
- File IO
  - Opens whatever path is specified in main library function
    iniparser_load() - appears to have relatively strict validation of
    input from this
- No logging
- No environment variable usage
- No use of privileged functions
- No use of cryptography / random number sources etc
- No use of temp files other than in unit tests
- No use of networking
- No use of WebKit
- No use of PolicyKit

- No significant cppcheck results
- No significant Coverity results
- No significant shellcheck results
- No significant bandit results

In general iniparser appears reasonably safe and the upstream seems
relatively responsive to potential security issues. The code has reasonable
unit tests...

Ok, thanks - thereby this seems to be ready.
Marking as such.

Here is a debdiff incorporating https://github.com/ndevilla/iniparser/commit/2412f165bcfde4ad8e3426fd59f2a920492b8c19 and the previous LP: #1915866 test build patch.

man dh_clean:
path ...
           Delete these paths too.

           Note that directories passed as arguments must end with a trailing slash. Any
           content in these directories will be removed as well.

i don't think obj-$() without '/' at the end will work.

Normally in override_dh_clean, dh_clean must be called last, otherwise it will wipe the state with the subsequent commands failing, and not remembering that it needs to rerun them. Thus it's always best to do

override_dh_clean:
    random stuff
    dh_clean

Also i'd rather you clean the test dir in "override_dh_auto_clean:" before calling regular dh_auto_clean. As that's the target meant for cleaning up the upstream build system mess.

Also need to ignore result of that make call, i.e. "-make clean"

Here is a v2 version which doesn't rely on debian/rules override.

Override component to main
iniparser 4.1-4 in hirsute: universe/misc -> main
libiniparser-dev 4.1-4 in hirsute amd64: universe/libdevel/optional/100% -> main
libiniparser-dev 4.1-4 in hirsute arm64: universe/libdevel/optional/100% -> main
libiniparser-dev 4.1-4 in hirsute armhf: universe/libdevel/optional/100% -> main
libiniparser-dev 4.1-4 in hirsute ppc64el: universe/libdevel/optional/100% -> main
libiniparser-dev 4.1-4 in hirsute riscv64: universe/libdevel/optional/100% -> main
libiniparser-dev 4.1-4 in hirsute s390x: universe/libdevel/optional/100% -> main
libiniparser-doc 4.1-4 in hirsute amd64: universe/doc/optional/100% -> main
libiniparser-doc 4.1-4 in hirsute arm64: universe/doc/optional/100% -> main
libiniparser-doc 4.1-4 in hirsute armhf: universe/doc/optional/100% -> main
libiniparser-doc 4.1-4 in hirsute i386: universe/doc/optional/100% -> main
libiniparser-doc 4.1-4 in hirsute ppc64el: universe/doc/optional/100% -> main
libiniparser-doc 4.1-4 in hirsute riscv64: universe/doc/optional/100% -> main
libiniparser-doc 4.1-4 in hirsute s390x: universe/doc/optional/100% -> main
libiniparser1 4.1-4 in hirsute amd64: universe/libs/optional/100% -> main
libiniparser1 4.1-4 in hirsute arm64: universe/libs/optional/100% -> main
libiniparser1 4.1-4 in hirsute armhf: universe/libs/optional/100% -> main
libiniparser1 4.1-4 in hirsute ppc64el: universe/libs/optional/100% -> main
libiniparser1 4.1-4 in hirsute riscv64: universe/libs/optional/100% -> main
libiniparser1 4.1-4 in hirsute s390x: universe/libs/optional/100% -> main
20 publications overridden.