handle_rx() dispatched to igmp_packet_handler() on the destination
address alone, so an ordinary IGMPv3 report off the wire reached the
handler and could write a table entry whether or not anyone had asked
for snooping. The STP branch right above it is gated on stpEnabled;
this brings the IGMP branch in line.
Snooping state lived only in the per-port registers, and the receive
path cannot afford to read one per packet, so the flag shadows it:
igmp_enable() sets it, igmp_setup() clears it, and igmp_setup() runs
from both the boot path and "igmp off".
While here, igmp off becomes an explicit subcommand instead of the
fall-through, and an unrecognised igmp subcommand prints the usage
line rather than silently turning snooping off.
Six bytes of BANK1 and one of xdata, no internal RAM.
l2.js reads the last index of a reply to know where the next page starts.
With the firmware side of this branch an empty table answers [], and
s[s.length-1] then throws on undefined. It used to answer commas with
nothing between them and throw in JSON.parse instead, so this is the same
case reaching a different line rather than a new one. An empty reply now renders what
has been collected and starts the next pass from zero.
The 4096 entry cap compared the array against the number instead of its
length, so it never fired: an empty array and a 5000 element one both
compare false. Comparing the length restores what the check was for.
Wrapping REG_SET restored the guard in front of the hash default, and
that exposed three things about the line it guards.
The test was against zero. The register does not read zero: it comes out
of reset holding source port number plus both MAC fields, both IP fields
and the L4 source port, which the header now names LAG_HASH_RESET.
Measured on an SWTGW218AS, where all four groups read 0x3f after a cold
boot and a value written before a power cycle is gone afterwards. With
the guard working and the test unreachable, the default would never be
installed, where before it was installed on every call. Testing against
the reset value restores the intent, and zero is still accepted in case
another device does reset that way.
The write went to the base address while the read that decides it used
the group offset, so a group other than zero was tested and group zero
was written. Both ends use the offset now.
The range check printed a complaint and carried on. It returns, which
matters more now that the hash write also uses the group number to build
an address.
Thirty bytes of BANK1.
The handler took one raw character of the request line and passed it to
send_counters(), which uses it to index machine.phys_to_log_port. That
array has nine entries and the character is whatever the client sent, so
the read ran up to 246 entries past the end and the result went on to
STAT_GET as a port number. is_word() accepts any request whose name is
followed by a question mark, so nothing constrained the byte to a digit.
Bounding it where it is read keeps the check beside the assumption it
protects and needs nothing from the machine description. sdcc leaves
plain char unsigned and the subtraction wraps in eight bits, so a byte
below '0' comes out above 200 and one upper test covers both ends:
exactly '0' to '8' now reach send_counters. The compiled test is
add a,#0xf7 followed by jnc, which I read back out of the assembly rather
than assuming.
Out of range answers 400 by the path the other malformed requests already
take, rather than an empty array. An empty array would have been worse
than useless here, since the statistics page calls BigInt on the first
element before it looks at the length. The page asks only for index zero
to the port count minus one, so nothing that answered before stops
answering, and a non-200 reply makes its handler do nothing at all.
11 bytes of BANK1, nothing in the common segment, BANK2, xdata or
internal RAM. Built for SWTGW218AS and KP_9000_6XHML_X2 on sdcc 4.5.0.
igmp_setup() writes the per-port IGMP configuration twice, once with the
value spelled out and once with the same number assembled from the
constants: IGMP_MAX_GROUP | IGMP_PROTOCOL_ENABLE | IGMP_FLOOD is exactly
0x00ff7c15. The second loop carries the comment block explaining the bit
layout, so the literal one is the one to drop.
Today the cost is one redundant register write, because REG_SET is not a
single statement and the unbraced loop body only ever reaches index
machine.max_port + 1. Once the macro is wrapped it becomes one redundant
write per port on every boot and on every "igmp off", which is what makes
this worth removing rather than leaving.
Fifty nine bytes of BANK1 on SWTGW218AS, nothing anywhere else.
Both macros expand to a run of statements joined by backslashes with
nothing around them, so as the unbraced body of an if or a for only the
first assignment belongs to that body. The other three and the
reg_write() call sit after it and run once, unconditionally, with
whatever the loop counter ended on. Wrapping each macro into a single
statement is what every call site already assumes it to be.
This hands the compiler no new room around the SFR writes, which is worth
showing rather than asserting. Building the whole image before and after
and comparing the generated assembly module by module, with label
numbering, block scope suffixes and the version string normalised away,
three modules differ: rtl837x_igmp, rtl837x_port and rtl837x_leds. Ten
modules call these macros, so the other seven come out identical, and so
does everything else in the image.
Those three differ because they hold the five unbraced uses. Two of them
want a fix rather than only the brace, and that is left to the commits
that follow.
Six bytes of BANK1 on SWTGW218AS, nothing anywhere else.
The POST path tests for a NUL before it looks at a byte. The GET path did not,
and is_separator() counts only space, tab, question mark and equals, so a request
line carrying none of those walks past the end of uip_buf and writes its
terminator into whatever xdata it happens to stop on.
Everything that is not a POST reaches that walk. The pointer advances past the
method before anything checks that the method was GET, so a TLS record sent to
port 80 by a browser trying https first is enough on its own, as is a port
scanner or a malformed line. The stop is wherever the first space, tab, question
mark or equals turns up in memory, which is why the symptoms are erratic.
Two bytes of BANK1. BANK2 and xdata do not move.
Review feedback on #306. The value used to ride in /information.json and
the picker fetched /vlanlist separately, so the page needed both requests
to mean anything. /vlanlist now answers {"mgmt":N,"vlan":[...]} and the
picker reads both from the one response. Both consumers in vlan.js were
taught the new shape, and /information.json no longer carries mgmt_vlan.
The truncation guard now reserves 141 bytes instead of 139: the closing
grew to two bytes with the wrapping object, and the comma in front of a
non-first entry was never counted, so the worst case could land one byte
past outbuf even before this change. char_to_html() does not check.
The comments added on this branch are gone as well, style.css and
system.js both, since these files are served byte for byte.
page_impl.rel stays at DSEG 5, OSEG 0, BSEG 3 and the image reports the
same 10183 bytes of XDATA before and after.
`vlan <id> mgmt` existed on the CLI, but nothing reported which VLAN
currently carries management, so the setting was invisible from the web UI -
the only way to find out was to read the startup config.
Report it in information.json and offer it where the other switch-wide
addressing settings live, between Gateway and Language, as a picker filled
from the configured VLANs. When management is untagged there is no VLAN to
select, so that state shows as a disabled entry rather than inventing an id
the switch would reject.
Confirm before applying, and say what will happen rather than echoing the
action back: the switch starts tagging its own frames, and if the port you
came in on does not carry that VLAN the page becomes unreachable and the way
back is the console. Cancelling puts the picker back where it was.
The System Settings rows all pair a label with a form control, except Model,
which is a bare span. The inputs carry padding and a left margin, so their
text starts about 32px further in than the model name did - close enough to
look like a mistake and far enough to see it.
Give read-only values a class with the same metrics instead of styling the
model specifically; any other value shown without a control gets the
alignment for free.
Review feedback on #307: these files are served from flash byte for byte,
so comments ride along on every page load. The three added in this branch
are gone, 357 bytes across l2.js and style.css. The column list existed in
three copies inside renderL2() and is now a single const.
Both handlers turned the first character of the port word into an index
with no check at all. "mtu 0 100" computes '0' - '1' = 255, reads far past
the end of phys_to_log_port[9], and writes the size to whatever register
0x1250 plus that garbage points at. "pvid 0 2" walks the same path into
port_pvid_set(). lag and vlan already validate their port arguments; these
two just did not.
The port now has to be a single digit 1 to 9, which is exactly the range
the mapping table holds. A second digit or a stray letter falls to the
usage message.
"mtu show" also gained the return it was missing: after printing the table
it fell through, derived a port from the word "show" and printed the
garbage byte before the length check stopped it.
"vlan 5000 mgmt" parked the management interface on a VLAN the table
cannot hold, which quietly cuts management off. IDs above 4094 now fall
through to the usage message. 0 still switches the management VLAN off,
which is the documented way to disable it.
"pvid 1 5000" packed 5000 into the 12-bit PVID field and truncated on the
way, so the port ended up with a PVID nobody chose. 0 and anything above
4094 are refused now, matching what the VLAN table can hold. A failed
parse used to be dropped without a word; both cases print the usage line.
The handler threw away atoi_short()'s return and leant on the range test
alone. The range test cannot tell a failed parse from a small number, so
"mtu 1 99999" stopped at the partial 9999 and went through as a number
nobody typed. With the parse result checked, a failure is rejected with
the same message as an out-of-range value.
"mtu 1 0" was accepted. The chip takes it verbatim, the port reports 0x0000
back, and it stops passing frames: on a live 2.5G LAG member the LACPDU
receives moved by 4 in fifteen seconds against 21 on the sibling port, and
the partner went expired. Restoring the size brought both back.
Nothing shorter than a minimum Ethernet frame is a usable maximum, so the
range is now 64 to 16383. The upper end is unchanged and still comes from
the width of the field the value is written into.
This also covers most of #312 by accident: "mtu 1 abc" leaves the parse
result at 0 and now gets rejected on the bound rather than reaching the
register. It does not cover all of it. The handler still ignores what
atoi_short() returns, so "mtu 1 99999" stops on a partial 9999 and goes
through as a number nobody typed.
The GUI is not affected either way, it offers a fixed list of sizes.
vlan_create() and vlan_delete() wrote the ID straight into the table index
register. vlan_get() has refused anything >= 0xfff for a while, so reads were
guarded and writes were not: "vlan 4095 1 2" built an entry that no read path
can see, and IDs above that either miss the table or alias onto another VLAN.
Both writers now enforce the range vlan_get() already did, and parse_vlan()
rejects the same values with the usage message so the CLI says why. The check
sits after the "vlan 0 mgmt" branch, which legitimately takes 0 to switch the
management VLAN off.
Costs nothing in RAM: rtl837x_port.rel stays at DSEG 0, OSEG 5, and the image
still reports 10207 bytes of XDATA in use.
atoi_short() accumulated into a uint16_t without checking, so "vlan 65540"
wrapped to 4 and edited VLAN 4 instead of failing. atoi_byte() had the same
hole with "300" landing on 44. Both now refuse the digit that would push the
value past its type, before it lands.
The partial result is deliberately left alone rather than zeroed. Zeroing
would give the function one tidy rule, every failure leaves 0, but a caller
that ignores the return would then write that 0, and 0 is not a harmless
number everywhere. Set as a port MTU it stops the port taking frames: I put
0 on a live 2.5G LAG member and its LACPDU receives moved by 4 in fifteen
seconds against 21 on the sibling port, with the partner going expired.
Putting the size back recovered both. A wrong number does less damage than
that, and the real fix belongs in the callers that ignore the return anyway.
The test sits inside the loop rather than after it, so no wider accumulator
is needed and the parser stays off the internal RAM budget.
056a30a on the branch in #303 fixes atoi_byte a different way, by widening
the accumulator. Whichever lands first, the other hunk should go.
The paint loop reuses existing rows and rewrites port, MAC, VLAN and the
delete button, but never the type cell - only the insert path set it. With a
fixed row order that stayed invisible, since a row usually landed back where
it was. Sorting moves rows, so every other column followed the data while
type kept the previous row's value, which made sorting by type look broken
when the sort itself was correct.
Clicking a heading sorted the table but nothing said so afterwards - the
only feedback was the rows moving, which is no help when the sort key is a
column you are not looking at.
Give every sortable heading a permanent marker: a neutral double arrow when
it is not the sort key, up or down when it is. The marker sits in its own
span so the i18n pass, which replaces the heading text, does not wipe it.
The table lists every learned and static entry in one flat block, which is
fine with a handful and unusable with a few hundred: finding out where one
MAC sits, or what a port has learned, meant reading the whole thing.
Make the column headings sort and give each one a filter box. Filters are
substring matches combined with AND, so "port 8 + static" is two keystrokes.
A counter above the table shows matched out of total, so a filter that hides
everything is obvious rather than looking like an empty table.
Sorting keeps the CPU entry from comparing as a number - it sorts last
instead of landing between ports 8 and 9, where a string-vs-number compare
would otherwise put it.
The filter inputs live inside the existing header cells rather than in a
second row: the paint loop addresses data rows as rows[i+1], and a second
header row would have shifted every one of them.
The port column was mapped from logical to physical numbering first and the
CPU label applied afterwards, testing for port 9. On an 8+1 board the SFP
port maps to physical 9 as well, so every entry learned on the SFP was shown
as CPU - on this switch that was 26 of 30 entries.
Label the CPU while the number is still logical, before the mapping, so the
two cannot collide.
Move the MAC-derived default name out of main() into its own function and
call it after execute_config(), returning early when the config already
set a name - a configured switch then does no work for it at all.
The body deliberately has no local variables. Locals here - counters and
pointers alike - land in the 8051's internal-RAM overlay, and on an image
with LACP and STP both enabled that overlay is exhausted: a loop makes the
linker fail with "Could not get 8 consecutive bytes in internal RAM for
area OSEG". Moving the code into its own function does not help, since the
overlay is shared across the whole image, and hoisting the locals to xdata
does not either, because itohex() is inline and brings its own frame. This
only shows up in an integrated build; the branch on its own links fine.
Suggested-by: vDorst
The hostname and model fields were populated through element-existence
checks and empty-string fallbacks that the other fields on the page do
without. Assign them the same way.
Suggested-by: vDorst
Every switch came up as "RTLPlayground", so several of them on one
network were indistinguishable until someone configured a name. Append
the last three MAC octets (e.g. RTLPlayground-1ef924) - unique in
practice, still recognisable, and any "hostname ..." line in the startup
config overrides it as before.
Suggested-by: plaes
Typing "hostname" on its own cleared the name: with no argument the copy
loop never ran and the terminating NUL landed at index 0. Report the
current name instead, accept exactly one argument to set it, and reject
anything longer - a name with spaces tokenizes into several words, and
silently keeping only the first one is worse than an error. Walk the
buffer with a pointer, which the compiler codes better than indexing.
Suggested-by: vDorst
machine.machine_name is already reported in /information.json as hw_ver;
surface it read-only on the System Settings page so the exact build
target is visible in the UI, without adding a redundant JSON field.
Add a device hostname settable from the CLI (`hostname <text>`) and the
System Settings page. The value is sanitized on ingest to JSON-safe
printable ASCII (<=23 chars), stored in a shared __xdata buffer, seeded
to "RTLPlayground" at boot, persisted through the startup-config, and
reported in /information.json. It lives in the common header so other
modules can advertise it (LLDP uses it as the System Name TLV).
Root cause of the "browser login always bounces back with Wrong password!
while curl works": scan_header() read the session id from a fixed offset
into the Cookie header (p + 17), assuming "session=" is the first and only
cookie. Browsers keep stale cookies for a long time - e.g. an "admin" cookie
left over from this switch's VENDOR firmware - so the header can arrive as
"Cookie: admin=..; session=..", the fixed offset then points into the admin
value, authentication silently fails and every page bounces to login although
the password had been accepted. curl sends only "session=", which is why
command-line tests passed while a real browser (with that stale cookie) failed.
- scan_header(): scan the Cookie header for the actual "session=" key
(matched as "session" - is_word() requires a separator after the pattern
and '=' is on its list, the first value byte is not).
- is_word_x(): accept ';' as a terminating separator so the session value
also matches when it is not the last cookie in the header.
Verified on hardware end-to-end in a real browser WITH the stale "admin"
cookie present: login -> index.html, all pages and JSON endpoints work.
Real root cause of the "web login fails from a browser but works from curl":
the uIP httpd is built with UIP_CONF_MAX_CONNECTIONS = 1 and uses global
response state (outbuf/slen/session), i.e. it serves exactly one TCP connection
at a time and closes it after each response - but never advertises that via
the Connection header. A browser's HTTP/1.1 client therefore assumes the
connection may be persistent and can park it in its keep-alive pool for reuse;
a later request sent on that pooled connection hits one the server has already
closed, and a POST (unlike a GET) is never retried by the browser, so it can
be silently lost this way.
This adds "Connection: close" to every response so the browser does not pool
and reuse a connection the server is about to drop. On its own this did not
fully explain the reported login failures - the actual authentication bug is
fixed in the next commit (the Cookie header parsed at a fixed offset) - but it
is correct behaviour for a server that only ever handles one connection, and
removes one source of dropped requests.
Two hygiene fixes for the web UI, prompted by a login that appeared to fail
under privacy shields (Brave Shields / NoScript-family extensions):
- httpd: replace the partial "style-src 'self' 'unsafe-inline'" CSP with a
complete, first-party policy (default-src 'self'; script-src 'self'
'unsafe-inline'; style-src 'self' 'unsafe-inline'; img-src 'self' data:;
connect-src 'self'; form-action 'self'). Everything the UI needs is
same-origin; the explicit policy stops shields injecting their own
restrictive report-only probes (the noisy script-src-elem 'none' console
spam) and passes a strict-CSP audit. Verified: no CSP violations in-browser.
- login.html: add autocomplete="current-password" so password managers
recognise the field (they showed "unknown password" without it).
NOTE: these do NOT bypass a browser's LAN-device protection (NoScript "lan" /
Brave Shields), which strips the POST body of requests to a LAN address and is
why the login can fail in-browser while the same credentials work over curl.
That is a deliberate browser security feature; the user must allow the site in
their shields to log in. The backend password (default 1234) is unchanged and
correct.
d00f