Fix bugs in esp32 mkimage.erl script #2093
Conversation
UncleGrumpy
force-pushed
the
mkimage-fix
branch
2 times, most recently
from
dc96177 to
ff8e506
Compare
UncleGrumpy
force-pushed
the
mkimage-fix
branch
from
ff8e506 to
91a7825
Compare
UncleGrumpy
force-pushed
the
mkimage-fix
branch
from
91a7825 to
83c100e
Compare
UncleGrumpy
force-pushed
the
mkimage-fix
branch
2 times, most recently
from
729ef65 to
89313e8
Compare
| SegmentEnd = | ||
| case try_read(SegmentPaths) of | ||
| {ok, Data} -> | ||
| file:write(Fout, Data), |
There was a problem hiding this comment.
From opus:
The return value of file:write(Fout, Data) is ignored. If the write fails (e.g., disk full), the script continues silently.
Fix: Change to ok = file:write(Fout, Data) to crash on error.
There was a problem hiding this comment.
This should cause a crash, as the result of file:write/2 will not be ok, so the script will not continue, but it will leave the file descriptor open, which is a problem.
(Edit: I did miss adding the ok = file:write(Fout, Data) here, I had fixed other occurrences, but missed that one. So it would NOT have crashed as it was)
There was a problem hiding this comment.
silently progessing doesn't seem good.. but nitpick
maybe try/after:
There was a problem hiding this comment.
I fixed this to throw a useful error message, and close the file descriptor when an error is encountered.
There was a problem hiding this comment.
try/after doesn't really apply here, errors from file:write/2 are returned as a tuple, not thrown.
There was a problem hiding this comment.
I couldn't view the screenshot, There is a message that the file is corrupted.
There was a problem hiding this comment.
I fixed this to throw a useful error message, and close the file descriptor when an error is encountered.
This had a bad line in it, it should be good now.
There was a problem hiding this comment.
try/after doesn't really apply here, errors from
file:write/2are returned as a tuple, not thrown.
but the after path is always run right? I'm no erlang expert - but seems to be DRY - but whatever you prefer!
There was a problem hiding this comment.
Yes, after is always run. That makes sense. Updated to use a single file:close/1 with try/after.
| if | ||
| Segments == [] -> | ||
| %% Pad to end on 32-byte alignment, since we don't have a next offset to pad too | ||
| SectorPad = 32 - (SegmentEnd rem 32), |
There was a problem hiding this comment.
From opus:
The padding calculation logic is slightly incorrect. 32 - (SegmentEnd rem 32) will result in 32 when SegmentEnd is perfectly aligned (remainder 0), causing 32 bytes of unnecessary padding to be written. The case 0 -> ok is unreachable.
Fix: Change the calculation to SectorPad = (32 - (SegmentEnd rem 32)) rem 32.
There was a problem hiding this comment.
Indeed. Although it could be argued that the next blocks should still be marked as free, using 16#ff, so there is a clear end to the segment, and any old data that may be present is not accidentally read as valid data. But the 0 case was not well thought out ;-)
There was a problem hiding this comment.
I cleaned this up and expanded the comment to explain padding with 32 bytes if the segment is already 32-byte aligned.
There was a problem hiding this comment.
I get a bad feeling with the implicit overpadding - of course it's hypotethical - but maybe I carefully craft a last segment that zeroes out everything, and then it's overpadded and img is too large, or some similar hypothetical..
There was a problem hiding this comment.
I did have that very same thought, there really isn’t a way to do that from within the ESP-IDF. What we really need here is to know either the length of the partition, or the offset of any partitions in the partition that follow the final image segment.
As it is right now, worst case is that we overwrite the first 32 bytes of the application partition, if the libraries in the boot partition happen to fill every single byte of the boot.avm partition exactly.
I can back out if the extra padding, but it there is any data already written to flash following the end of the esp32boot.avm libraries this may be silently loaded and processed by the VM.
There was a problem hiding this comment.
But… if we follow my suggestion of moving the NVS partition to follow the library partition, we may be overwriting values that the user intended to keep.
There was a problem hiding this comment.
I’m am inclined to rework the config, and use the offsets and partition lengths from the partitions.csv that the IDF is using for the partition table.
That should be our source of truth for all partition related data.
UncleGrumpy
force-pushed
the
mkimage-fix
branch
3 times, most recently
from
c0604b3 to
9faef56
Compare
* Fixes an FD leak, the image file was never closed properly after writing. * Fixes a misleading error message when a binary overflows its partition. * Avoids possibly reading possibly corrupt old data beyond the end of the last segment written by padding the remainder of the last sector (flash sectors are 4096 blocks in length) with 0xff. Having this buffer marked as erased between the end of the partition data and any bits left on the device from previously flashed projects should prevent accidentally ace ping this as part of the partition data. Also fixes and unaligned end of data problems that can prevent flashing with a web-flasher tool. * Now catches errors in file:write/2 operations, rather than blindly continue when an error is returned. Closes atomvm#2084 Closes atomvm#2094 Signed-off-by: Winford <winford@object.stream>
UncleGrumpy
force-pushed
the
mkimage-fix
branch
from
9faef56 to
6913849
Compare
2 participants
Fixes an FD leak, the image file was never closed properly after writing. Fixes a misleading error message when a binary overflows its partition. Catches errors in file:write/2 operations, rather than blindly continuing.
Closes #2084
Closes #2094
These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).
SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later