Skip to content

Conversation

TimJTi
Copy link
Contributor

@TimJTi TimJTi commented Jul 2, 2025

Summary

When trying to enable and use the thttp net utility, it was found that CGI apps didnt send any http responses of data from the CGI app back to the browser client,

Investigation revealed this was due to the net socket being opened with the O_CLOEXEC flag set.

This is the result, I believe, of a PR last year here.

See this issue.

In correcting the issue, some complier warnings were encountered and fixed, along with a Kconfig error and an omission, and the opportunity was taken to improve some help text within Kconfig.

This is why there are two commits in the PR.

Question

The cgi_child app that is spawned closes all open file descriprors except stdin, stdout, stderr and the socket's descriptor, up to a maximum number of file descriptors as set via Kconfig.

See here.

Should the Kconfig option be removed and the function use OPEN_MAX instead?

Impact

No impact on existing usage of this netutil, other than to allow it work properly now!

Testing

This has been tested as part of my own application development, using a custom SAMA5D27C-D1G processor, with the webserver running via CDC-NCM.

@TimJTi TimJTi force-pushed the netutils/thttpd-fix-broken-CGI-and-fixup-Kconfig branch from e5fba51 to e53907d Compare July 2, 2025 11:43
Copy link
Contributor

@cederom cederom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @TimJTi :-)

@xiaoxiang781216
Copy link
Contributor

@TimJTi please fix nuttx side:

====================================================================================
Configuration/Tool: lincoln60/thttpd-binfs,CONFIG_ARM_TOOLCHAIN_GNU_EABI
2025-07-02 11:49:28
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Disabling CONFIG_ARM_TOOLCHAIN_GNU_EABI
  Enabling CONFIG_ARM_TOOLCHAIN_GNU_EABI
  Building NuttX...
  [1/1] Normalize lincoln60/thttpd-binfs
64d63
< CONFIG_THTTPD_CGIINBUFFERSIZ=256
Saving the new configuration file
On branch master
Your branch is up to date with 'origin/master'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   boards/arm/lpc17xx_40xx/lincoln60/configs/thttpd-binfs/defconfig

@TimJTi
Copy link
Contributor Author

TimJTi commented Jul 2, 2025

@TimJTi please fix nuttx side:

====================================================================================
Configuration/Tool: lincoln60/thttpd-binfs,CONFIG_ARM_TOOLCHAIN_GNU_EABI
2025-07-02 11:49:28
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Disabling CONFIG_ARM_TOOLCHAIN_GNU_EABI
  Enabling CONFIG_ARM_TOOLCHAIN_GNU_EABI
  Building NuttX...
  [1/1] Normalize lincoln60/thttpd-binfs
64d63
< CONFIG_THTTPD_CGIINBUFFERSIZ=256
Saving the new configuration file
On branch master
Your branch is up to date with 'origin/master'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   boards/arm/lpc17xx_40xx/lincoln60/configs/thttpd-binfs/defconfig

@xiaoxiang781216
Oops!

Can I do it as a third commit in this PR or do I need to do it via a new PR in the nuttx repo?

@cederom
Copy link
Contributor

cederom commented Jul 2, 2025

Different repo different commit/pr :-)

@xiaoxiang781216
Copy link
Contributor

@TimJTi please fix nuttx side:

====================================================================================
Configuration/Tool: lincoln60/thttpd-binfs,CONFIG_ARM_TOOLCHAIN_GNU_EABI
2025-07-02 11:49:28
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Disabling CONFIG_ARM_TOOLCHAIN_GNU_EABI
  Enabling CONFIG_ARM_TOOLCHAIN_GNU_EABI
  Building NuttX...
  [1/1] Normalize lincoln60/thttpd-binfs
64d63
< CONFIG_THTTPD_CGIINBUFFERSIZ=256
Saving the new configuration file
On branch master
Your branch is up to date with 'origin/master'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   boards/arm/lpc17xx_40xx/lincoln60/configs/thttpd-binfs/defconfig

@xiaoxiang781216 Oops!

Can I do it as a third commit in this PR or do I need to do it via a new PR in the nuttx repo?

please provide pr to nuttx git, I will merge both patch together.

@xiaoxiang781216 xiaoxiang781216 marked this pull request as ready for review July 2, 2025 18:06
@xiaoxiang781216 xiaoxiang781216 merged commit e6a335d into apache:master Jul 3, 2025
15 of 39 checks passed
@TimJTi
Copy link
Contributor Author

TimJTi commented Jul 3, 2025

@xiaoxiang781216
I'm still seeing the compiler warnings that were fixed in one of the commits here. I wonder if the merge sequence was such that the warnings were "undone" by the actual real fix?

Please advise if you need me to do anything to sort this as I'm sure it was down to me LOL

@xiaoxiang781216
Copy link
Contributor

@xiaoxiang781216 I'm still seeing the compiler warnings that were fixed in one of the commits here. I wonder if the merge sequence was such that the warnings were "undone" by the actual real fix?

Please advise if you need me to do anything to sort this as I'm sure it was down to me LOL

let's see what' happen when the new PR come in.

@TimJTi
Copy link
Contributor Author

TimJTi commented Jul 3, 2025

@xiaoxiang781216 I'm still seeing the compiler warnings that were fixed in one of the commits here. I wonder if the merge sequence was such that the warnings were "undone" by the actual real fix?
Please advise if you need me to do anything to sort this as I'm sure it was down to me LOL

let's see what' happen when the new PR come in.

@xiaoxiang781216 - what new PR?

@xiaoxiang781216
Copy link
Contributor

If the new PR doesn't have the build break, your change is fine

@TimJTi
Copy link
Contributor Author

TimJTi commented Jul 4, 2025

If the new PR doesn't have the build break, your change is fine

Sorry - I hadn't properly rebased by branch to latest upstream so it had a mix of upstream/master and my own changes that were conflicted. All is good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] netutils/thttpd CGI broken - incorrect use or changed behaviour of exec()?
4 participants